Skip to content

Conversation

@pancak3
Copy link
Contributor

@pancak3 pancak3 commented Oct 4, 2025

/resolve #290

@pancak3
Copy link
Contributor Author

pancak3 commented Oct 4, 2025

Hey @maugustosilva @Vezio, this is sadly a huge pr :(

Although I tried my best to validate changes locally, I must not have covered many things due to environment limits. Testing this PR in your environment should be required as well. Please let me know what issues, and enjoy the mess : D

In general, this PR includes pdm as the package manager. Ruff is configured as the formatter and linter. The main source files are moved to the folder llm_d_benchmark under the root. The following are the related changes I made:

  1. Updated main readme
  2. Removed installations of py deps from install_deps.sh since pdm manages them
  3. Fixed pre-commit
  4. Added formatter and linter in pre-commit hooks
  5. Removed unnecessary go entries from the makefile
  6. Fixed bugs that do not respect dry-run in some steps
  7. Changed & fixed Python imports
  8. Added formatter and linter in workflow
  9. Changed necessary paths in the Dockerfile. The container image was built successfully, but I doubt the Dockerfile has some bugs. I will pass this to the person who owns it
  10. others, see changes

Most of the py file changes are from the formatter and linter and are thus safe to skip in your review.

Quick check cmds

pip install pdm && pdm install && pdm run $SHELL

pdm run format-check
pdm run lint-check
pdm run test

pdm run pre-commit run --all 

Cheers.

@pancak3 pancak3 marked this pull request as ready for review October 4, 2025 13:34
@maugustosilva maugustosilva requested a review from Vezio October 6, 2025 14:50
@Vezio Vezio requested review from jgchn and maugustosilva October 6, 2025 16:07
@maugustosilva
Copy link
Collaborator

@pancak3 Oh well :-). First of all thanks for the contributions, but with 165 files changed, I need to ask about the possibility of splitting it on smaller PRs.

@Vezio
Copy link
Collaborator

Vezio commented Oct 6, 2025

@pancak3 I'd propose the following integration path:

  • Open a PR with changes for integrating the linter to automation (but not directly using it, yet)
  • Open a PR with changes for the changes made by the linter (then enable the linter automation)
    • For this one - I'd prefer if we made the changes in chunks so we can transparently view what was changed
  • Open a PR with changes for the Python Package restructure

Thank you for such great efforts!

@pancak3
Copy link
Contributor Author

pancak3 commented Oct 7, 2025

@pancak3 I'd propose the following integration path:

  • Open a PR with changes for integrating the linter to automation (but not directly using it, yet)

  • Open a PR with changes for the changes made by the linter (then enable the linter automation)

    • For this one - I'd prefer if we made the changes in chunks so we can transparently view what was changed
  • Open a PR with changes for the Python Package restructure

Thank you for such great efforts!

That sounds pretty reasonable to me. I will check out the first todo item within the week and others after. Got some research ddls to catch this month. Thanks! @Vezio @maugustosilva

@pancak3 pancak3 marked this pull request as draft October 7, 2025 13:18
@Vezio
Copy link
Collaborator

Vezio commented Oct 7, 2025

@pancak3 awesome thanks! I sense there will be a LOT of contributions this week and a bit of refactoring, so it will probably be good timing for your research you have going on so the code takes a breather 😄

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.

Include pylint

3 participants