Skip to content

Conversation

@AlonKellner-RedHat
Copy link
Collaborator

@AlonKellner-RedHat AlonKellner-RedHat commented Oct 30, 2025

Summary

E2E tests which check basic GuideLLM functionality, using vLLM simulator.

Details

Test Plan

  • Local testing
  • GitHub action

  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes AI-assisted code completion
  • Includes code generated by an AI application
  • Includes AI-generated tests (NOTE: AI written tests should have a docstring that includes ## WRITTEN BY AI ##)

Copy link
Collaborator

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good overall. It is fairly self-contained, so it shouldn't cause any issues as long as the tests aren't flaky. Just one blocker as discussed in the call.

Comment on lines 29 to 33
uv self update
uvx --version
echo python
uv python install 3.12 --default
python --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

During our call, we concluded that it would likely be a better practice to use the pylock to set dependencies in the E2E tests.

Copy link
Collaborator Author

@AlonKellner-RedHat AlonKellner-RedHat Nov 5, 2025

Choose a reason for hiding this comment

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

@jaredoconnell Unfortunately it ended up as a change to tox usage all across the board - since we never actually used the pylock.toml in our tests.
I added a tox plugin called tox-pdm which uses our lock file, then I integrated that modification to all tox related workflows.
From now on - every single tox based test environment will use pdm and pylock.toml by default.

Copy link
Collaborator

@sjmonson sjmonson left a comment

Choose a reason for hiding this comment

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

Few small nits

@AlonKellner-RedHat
Copy link
Collaborator Author

@sjmonson I just straight up applied all of your suggestions.
@jaredoconnell I removed the now redundant aiologic dependency, and integrated tox-pdm with all tox based tests.
Anything else you want to tackle before we land this one?

Copy link
Collaborator

@sjmonson sjmonson left a comment

Choose a reason for hiding this comment

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

One more nit. Also needs signoff, E.g. git rebase --signoff main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert this. It resolves to python>=3.10,<4.0, python>=3.11,<4.0, python>=3.12,<4.0; overlapping ranges do not make sense to lock separately. The previous version was python>=3.12,<4.0, python>=3.10,<3.12. Having a split is not strictly necessary anymore since we dropped 3.9 but it does allow us to lock newer packages for the modern python version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I can't really fully revert this, because the original generated pylock.toml does not work for 3.10 (for example torchcodec revolves only 3.12 and 3.13), and the CI fails.
I am working on modifying my version to generate non-overlapping ranges that still work for both 3.10 and 3.13 to pass the CI. Will update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK I see the issue - I was using the generate_pylock.sh script without -f which generated an invalid pylock.toml (not sure exactly why, seems like a strange interaction between --append and --update-reuse).
In any case, I am applying a full revert of my changes

AlonKellner-RedHat and others added 21 commits November 6, 2025 10:57
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
tukwila and others added 15 commits November 6, 2025 10:57
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: guangli.bao <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: dalthecow <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
Signed-off-by: dalthecow <[email protected]>
Signed-off-by: Alon Kellner <[email protected]>
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.

6 participants