Skip to content

Clean up examples and PR template #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 12, 2025
Merged

Conversation

rafvasq
Copy link
Collaborator

@rafvasq rafvasq commented Jun 11, 2025

  • Tidying up the python examples with clearer names and directories
  • Moving the PR template which seems like it should be in the root of .github/ to be seen

rafvasq added 2 commits June 11, 2025 12:26
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Rafael Vasquez <[email protected]>
Copy link

👋 Hi! Thank you for contributing to vLLM support on Spyre.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, first install the linting requirements, then run format.sh and commit the changes. This can be done with uv directly:

uv sync --frozen --group lint --active --inexact

Or this can be done with pip:

uv pip compile --group lint > requirements-lint.txt
pip install -r requirements-lint.txt
bash format.sh

Now you are good to go 🚀

rafvasq added 3 commits June 11, 2025 13:17
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Rafael Vasquez <[email protected]>
Copy link
Collaborator

@prashantgupta24 prashantgupta24 Jun 11, 2025

Choose a reason for hiding this comment

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

These changes might have a major conflict on the change that Sophie is making in this PR - #228

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes are compatible 👍but you will probably need to handle a couple of conflicts

@prashantgupta24
Copy link
Collaborator

Apart from the upcoming conflict, I love the changes! I can wait for both of you to coordinate with each other to see who will pick up whose changes before approving this PR

@sducouedic
Copy link
Collaborator

it's good to see these simplifications

Question: should the notebooks (.ipynb) files also be moved to offline_inference and online_inference ?

Otherwise LGTM

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes are compatible 👍but you will probably need to handle a couple of conflicts

Signed-off-by: Rafael Vasquez <[email protected]>
@rafvasq
Copy link
Collaborator Author

rafvasq commented Jun 12, 2025

it's good to see these simplifications

Question: should the notebooks (.ipynb) files also be moved to offline_inference and online_inference ?

Otherwise LGTM

I think so! Or maybe a examples/notebooks/ dir. But I can take care of them in another PR cleaning them up as well.

@rafvasq rafvasq requested a review from prashantgupta24 June 12, 2025 14:24
Copy link
Collaborator

@prashantgupta24 prashantgupta24 left a comment

Choose a reason for hiding this comment

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

LGTM

@rafvasq rafvasq merged commit 97d03d6 into vllm-project:main Jun 12, 2025
20 checks passed
@rafvasq rafvasq deleted the clean-examples branch June 12, 2025 18:59
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.

3 participants