fix: handle inline comments in requirements.txt parsing#433
Merged
Conversation
Replace the per-line pip install loop in execute_install_script with `pip install -r <path>`, delegating requirements parsing to pip. This fixes inline comments being passed verbatim to pip (Invalid requirement errors) and incidentally handles line continuations, -e, --index-url, env markers, and hash specs. Also strip inline comments (matching pip's COMMENT_RE regex) in: - parse_req_file in uv.py, which feeds pip download/wheel argv. - initialize_project_config in registry/config_parser.py, which produces pyproject.toml [project.dependencies]; additionally skip pip-only options (-r, -e, --index-url, --find-links) that are not valid PEP 508 specifiers and would corrupt the generated TOML. The regex (^|\s+)#.*$ matches pip's exact rule: `#` only starts a comment when preceded by whitespace, so VCS URL fragments like Signed-off-by: Alexander Piskun <bigcat88@icloud.com> #subdirectory=pkg and direct-URL #sha256= hashes survive.
dosubot
Bot
added
size:M
📝 WalkthroughWalkthroughReplaces per-line pip installs with a single Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #433 +/- ##
==========================================
+ Coverage 77.15% 77.47% +0.32%
==========================================
Files 35 35
Lines 4311 4316 +5
==========================================
+ Hits 3326 3344 +18
+ Misses 985 972 -13
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/command/custom_nodes/command.py`:
- Around line 171-175: The pip install call builds install_cmd with the
possibly-relative requirements_path which, when combined with subprocess
cwd=repo_path in try_install_script, can produce a duplicated path; change the
code that constructs install_cmd (where install_cmd = [python, "-m", "pip",
"install", "-r", requirements_path]) to pass an absolute path for
requirements_path (e.g., compute abs_requirements =
os.path.abspath(requirements_path) or os.path.join(os.path.abspath(repo_path),
...) before creating install_cmd) so pip receives "-r" followed by an absolute
file path; update the creation of install_cmd in the block that uses
resolve_workspace_python, requirements_path and repo_path.
In `@comfy_cli/uv.py`:
- Around line 40-43: The duplicate pip-style comment regex (_inline_comment_re =
re.compile(r"(^|\s+)#.*$")) appears in this module and in the config parser;
extract it into a single shared helper (e.g., a small requirements-parsing
helper exposing INLINE_COMMENT_RE or get_inline_comment_re) and replace the
local _inline_comment_re definitions with an import of that symbol in both
places (the current uv module and the config parser module), add a short
docstring explaining the pip behavior, and run/update any tests that reference
the regex to ensure no behavioral change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0303f7e-ca8c-4162-8f9e-5238c36a663f
📒 Files selected for processing (6)
comfy_cli/command/custom_nodes/command.pycomfy_cli/registry/config_parser.pycomfy_cli/uv.pytests/comfy_cli/registry/test_config_parser.pytests/comfy_cli/test_custom_nodes_python_resolution.pytests/uv/test_uv.py
try_install_script runs pip with cwd=repo_path, so if requirements_path were relative it would get re-resolved against that cwd, producing a doubled path like <rel>/<rel>/requirements.txt. Wrap the requirements_path with os.path.abspath before handing it to pip to make the -r target unambiguous regardless of whether the caller passes an absolute or relative repo_path. Add a regression test that invokes execute_install_script with a relative repo_path. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Address review feedback: - initialize_project_config now prints a warning for each requirements.txt line it skips when generating [project.dependencies] (-r, -e, -c, --index-url, etc.). Previously these were silently dropped, leaving node authors to debug a missing dep in pyproject.toml with no signal. - Tighten test_inline_comment_not_passed_to_pip to assert the exact comment-laden raw line does not leak into pip argv, instead of a loose "no # in argv" check (which could false-fail if a path in argv ever happened to contain a #). - Add regression test for CRLF line endings in parse_req_file so Windows-authored requirements.txt files are covered. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/registry/config_parser.py`:
- Around line 266-268: Replace the lone print call that emits the pip-only
requirements warning with typer.echo to match the rest of this module's logging
style: change the print(...) that outputs f"Warning: skipping pip-only option
from requirements.txt (not valid as PEP 508 dep): {line!r}" to typer.echo(...)
and ensure typer is imported in config_parser.py (the surrounding helpers like
validate_and_extract_os_classifiers and validate_version use typer.echo, so
mirror that pattern).
In `@tests/uv/test_uv.py`:
- Around line 417-420: Add a short docstring to the test function
test_parse_req_file_preserves_double_dash_options explaining that parse_req_file
intentionally preserves double-dash CLI-style options (e.g.,
"--extra-index-url") for downstream pip consumption, and clarifying this is
distinct from the compile stage where DependencyCompiler (see
test_compiled_output_has_no_extra_index_url) strips such options from compiled
output; place the docstring at the top of
test_parse_req_file_preserves_double_dash_options so future readers understand
the different responsibilities of parse_req_file vs. DependencyCompiler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c787457b-d88b-4975-8ec2-a2de60271e3c
📒 Files selected for processing (4)
comfy_cli/registry/config_parser.pytests/comfy_cli/registry/test_config_parser.pytests/comfy_cli/test_custom_nodes_python_resolution.pytests/uv/test_uv.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #431 —
comfy node installon a custom node whoserequirements.txtcontains an inline comment (e.g.matplotlib>=3.3.0 # For visualization) fails withERROR: Invalid requirement: pip receives the whole line verbatim because comfy-cli parses the file itself and passes each line as a single argv element.Three locations in comfy-cli parse
requirements.txtby hand, all with the same bug class:execute_install_script(custom_nodes/command.py)pip install <spec>per lineparse_req_file(uv.py)download/wheelargvinitialize_project_config(registry/config_parser.py)requirements.txt→pyproject.toml [project.dependencies]Fix
pip install -r <path>— matches the four otherpip install -rcall sites already in the repo (install.py:110,128,cmdline.py:415,command.py:397). Fixes inline comments plus line continuations,-e,-r,--index-url,--hash, and environment markers for free.(^|\s+)#.*$. Critically,#is only a comment marker when preceded by whitespace, so VCS URL fragments likegit+https://host/r.git#subdirectory=pkgand direct-URL#sha256=hashes survive.-r,-e,--index-url,--find-links) that are not valid PEP 508 specifiers and would have corrupted[project.dependencies]in the generatedpyproject.toml.Why not
line.split("#")[0]?A naive
split("#")[0]silently breaks VCS URL fragments, turninggit+https://host/mono.git#subdirectory=pkgintogit+https://host/mono.git— pip would then fail to findsetup.pyat the monorepo root. Regression tests guard against this.Scope note
The reporter's traceback actually originates from ComfyUI-Manager's own
execute_install_script(manager_core.py), not comfy-cli — for git-clone nodes ComfyUI-Manager doesn't call back into comfy-cli. comfy-cli has the same class of bug in its own registry-install path, which this PR fixes. ComfyUI-Manager needs a separate patch upstream.Test plan
ruff check+ruff format --checkcleanpip._internal.req.req_file.COMMENT_REacross 23 tricky inputs (tabs, multiple spaces, VCS fragments, direct-URL hashes, decorative######, mid-string#without whitespace, env markers, line continuations)pip install --dry-run -r <file>verified to handle empty files, comment-only files, and the reporter's actual input shape[repo_path, *cmd]as a Python list literal; cm-cli consumers usingsubprocess.call(cmd[1:])verbatim are unaffected