Skip to content

fix: capture commented imports in Tool requirements#2536

Open
nolanchic wants to merge 1 commit into
huggingface:mainfrom
nolanchic:fix/tool-requirements-ast-imports
Open

fix: capture commented imports in Tool requirements#2536
nolanchic wants to merge 1 commit into
huggingface:mainfrom
nolanchic:fix/tool-requirements-ast-imports

Conversation

@nolanchic

Copy link
Copy Markdown

What does this PR do?

Closes #2211.

get_imports() extracted imported modules with a regex anchored at end-of-line (^\s*import\s+(\S+?)(?:\s+as\s+\S+)?\s*$). Any import carrying a trailing comment failed to match, so it was silently dropped — e.g. import IPython # noqa: F401 or import numpy as np # type: ignore. As a result Tool.save / Tool.to_dict produced an incomplete requirements.txt and saved tools could be missing dependencies.

How

Rewrite get_imports to parse the snippet with ast and walk the statements. Comments are ignored by construction, and multi-name imports (import a, b) are handled correctly. textwrap.dedent is applied first so indented snippets (e.g. a tool body) parse without an IndentationError.

The previous filtering behaviour is preserved:

  • imports inside try/except blocks (optional dependencies) are skipped,
  • imports inside if is_flash_attn_*_available() guards (cpu-only environments) are skipped,
  • relative imports (from . import x) are excluded.

Test

  • Added a parametrized case to test_get_imports covering import / import ... as / from ... import each followed by an inline comment. Fails on main, passes with this change.
  • All existing parametrized cases (try/except, flash-attn guard, relative imports, multi-line, import os.path) still pass.
  • End-to-end: a @tool whose forward contains a commented import now lists it in to_dict()["requirements"].

get_imports() used a regex anchored on end-of-line, so any import with a trailing comment (e.g. 'import IPython  # noqa: F401' or 'import numpy as np  # type: ignore') was silently dropped, and Tool.save emitted an incomplete requirements.txt.

Rewrite get_imports to walk the AST, which ignores comments by construction and also handles multi-name imports. The previous behaviour of skipping imports inside try/except blocks and is_flash_attn_*_available() guards is preserved, as is the filtering of relative imports.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tool save can miss requirements for commented imports

1 participant