Skip to content

[deps] raydepsets: lockfile parser#60839

Open
elliot-barn wants to merge 2 commits intomasterfrom
elliot-barn-pip-requirements-parser
Open

[deps] raydepsets: lockfile parser#60839
elliot-barn wants to merge 2 commits intomasterfrom
elliot-barn-pip-requirements-parser

Conversation

@elliot-barn
Copy link
Contributor

@elliot-barn elliot-barn commented Feb 7, 2026

  • Add a parser funcs to ci/raydepsets/cli.py that uses
    pip_requirements_parser to read and write pip lock files,
    providing parse_lock_file() and write_lock_file() utilities
  • Add pip_requirements_parser as a CI dependency in
    release/requirements_py310.in and recompile the lock file
    with Python 3.10
  • Prerequisite to relax operation in raydepsets

Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
@elliot-barn elliot-barn requested a review from aslonnie February 7, 2026 23:53
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new parser module for pip lock files using pip_requirements_parser, which is a good addition for managing dependencies in raydepsets. The overall structure, including the new module, tests, and Bazel configuration, is well-organized. However, I've identified a significant correctness issue in the parser implementation where it loses information like comments and options during a read-write cycle. I've provided a suggestion to make the parser lossless. Additionally, I've recommended strengthening the tests to better ensure the correctness of the parsing and writing logic. The dependency updates are appropriate for the new functionality.

Comment on lines 97 to 98
assert len(reqs) == len(reqs2)
assert [r.name for r in reqs] == [r.name for r in reqs2]
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The assertions in this roundtrip test are not very strong as they only check the number of packages and their names. This might not catch issues with version specifiers, hashes, or other requirement details being lost or modified during the write-read cycle.

A stronger assertion would be to compare the full requirement lines. For example:

assert [r.requirement_line.line for r in reqs] == [r.requirement_line.line for r in reqs2]

If you adopt my suggestion for parser.py, you could even compare the full serialized output, which would be an even better test of correctness:

# With suggested changes to parser.py
rf = parse_lock_file(str(lock_file))
output_file = Path(tmpdir) / "output.txt"
write_lock_file(rf, str(output_file))
rf2 = parse_lock_file(str(output_file))

assert rf.dumps() == rf2.dumps()

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
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.

1 participant