Skip to content

refactor: add missing explicit (non-dynamic) imports silence linter errors#719

Draft
anatoly-ryabchenko wants to merge 2 commits intoroboflow:developfrom
anatoly-ryabchenko:explicit-imports
Draft

refactor: add missing explicit (non-dynamic) imports silence linter errors#719
anatoly-ryabchenko wants to merge 2 commits intoroboflow:developfrom
anatoly-ryabchenko:explicit-imports

Conversation

@anatoly-ryabchenko
Copy link
Copy Markdown

@anatoly-ryabchenko anatoly-ryabchenko commented Feb 22, 2026

What does this PR do?

import ast was missing from main.py
import torch was missing from datasets/_develop.py
this may not have caused runtime crashes, since dependencies were imported upstream, but it is a bad practice and adds linter errors. This PR fixes that.

Related Issue(s): N/A

Type of Change

  • Refactoring (no functional changes)

Testing

  • I have tested this change locally
  • I have added/updated tests for this change

Test details:
Linter errors disappear (tested with PyCharm), functionality not affected (training, export).

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in hard-to-understand areas
  • My changes generate no new warnings or errors
  • I have updated the documentation accordingly (if applicable)

Additional Context

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65%. Comparing base (b8bcc3e) to head (8ed14e7).
⚠️ Report is 177 commits behind head on develop.

❌ Your project check has failed because the head coverage (65%) is below the target coverage (95%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@          Coverage Diff           @@
##           develop   #719   +/-   ##
======================================
  Coverage       65%    65%           
======================================
  Files           56     56           
  Lines         7208   7210    +2     
======================================
+ Hits          4708   4711    +3     
+ Misses        2500   2499    -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Borda
Copy link
Copy Markdown
Member

Borda commented Feb 22, 2026

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@anatoly-ryabchenko ^^

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds explicit imports for ast in main.py and torch in datasets/_develop.py to resolve linter errors. While these imports are technically used in both files, adding the module-level torch import in _develop.py conflicts with the intentional lazy-loading design of the _SimpleDataset class.

Changes:

  • Added import ast to src/rfdetr/main.py (used for ast.literal_eval in argument parsing)
  • Added import torch to src/rfdetr/datasets/_develop.py (needed for torch.Tensor type hint)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/rfdetr/main.py Adds missing ast import needed for ast.literal_eval call on line 1016
src/rfdetr/datasets/_develop.py Adds module-level torch import for type hint, but conflicts with lazy loading design

from typing import Any, Generator, Optional, Tuple
from urllib.request import urlretrieve

import torch
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a module-level torch import conflicts with the documented design intention of _SimpleDataset. The class docstring explicitly states that it "does not pull in torch at module load time" (lines 44-46), and torch was intentionally imported locally within __getitem__ (line 72) to achieve lazy loading.

While the module-level import is needed for linter compatibility with the torch.Tensor type hint on line 70, this breaks the lazy loading behavior. Consider one of these alternatives:

  1. Use a string literal for the type hint: def __getitem__(self, idx: int) -> Tuple["torch.Tensor", dict]: combined with from __future__ import annotations (already present)
  2. Import torch conditionally using TYPE_CHECKING: if TYPE_CHECKING: import torch
  3. Update the docstring to reflect that torch is now imported at module load time

Since this file already uses from __future__ import annotations (line 13), option 1 (string literal) would preserve the lazy loading behavior while satisfying linters.

Copilot uses AI. Check for mistakes.
@anatoly-ryabchenko
Copy link
Copy Markdown
Author

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@anatoly-ryabchenko ^^

I have accepted the license agreement several times, but it still shows up as not signed.

This is my first PR for an open-source project, so I'm a bit new to this.
For now, I will move it to draft, and will re-open it when the issues are fixed:

  1. Will check how Codecov checks coverage and why it's not passing.
  2. Will change the torch import to make it dynamic.
  3. Will also check how to cleanly fix other dynamic imports to not trigger linter.

@Borda
Copy link
Copy Markdown
Member

Borda commented Mar 24, 2026

I have accepted the license agreement several times, but it still shows up as not signed.

could you please share the screenshot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants