Skip to content

Various bits of cleanup #146

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Various bits of cleanup #146

wants to merge 13 commits into from

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 2, 2025

I was trying to run the benchmarks in the repo and found various small improvements that should help with being able to run these more out of the box:

  • The Makefile now supports running all the benchmarks without the virtual env if desired.
  • I removed the PYTHONPATH override, which prevents the various python -m commands from running since that relies on the current directory being on sys.path
  • I added support for passing the python executable as an argument so that the no-env jobs can reuse bits if they need to. For now, we're not using that since the only run target that depends on building the tables is run-polars, but if we want to change that so that all targets rebuild the tables this change would help
  • Changes the ifndef to an ifndef-else to remove warnings around multiple definitions of a target.
    Feel free to let me know if any of these changes seem undesirable for some reason that I am not aware of.

In the process I also improved docs/comments in a couple of places and fixed some formatting. I also deleted a file that looks like it was recently added accidentally in #144.

@vyasr vyasr requested a review from Matt711 April 18, 2025 16:51
@vyasr vyasr mentioned this pull request Apr 23, 2025
@vyasr
Copy link
Contributor Author

vyasr commented Apr 23, 2025

Based on some feedback in the Discord, I'll attempt to split this PR up into more easily reviewable chunks.

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.

2 participants