Skip to content
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

fix: Don't generate pyc files on installation #92

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

ewianda
Copy link
Contributor

@ewianda ewianda commented Apr 22, 2024

I experienced this error openai/tiktoken#152 and figured it is better to remove all pycache directories in general

@ewianda
Copy link
Contributor Author

ewianda commented Apr 24, 2024

ping @jvolkman. Can you please take a look

@jvolkman
Copy link
Owner

@ewianda are these __pycache__ directories in the wheel? Can you link to the problematic wheel? I'd like to better understand what's occurring here.

@ewianda
Copy link
Contributor Author

ewianda commented Apr 24, 2024

I think I missed diagnosing the problem, it doesn't seem like there are any pycache folders in the wheel. I believe the pycache folders are created when calling the installer.

@jvolkman
Copy link
Owner

Let's try setting bytecode_optimization_levels to an empty list. That should disable the generation.

https://github.com/jvolkman/rules_pycross/blob/main/pycross/private/tools/wheel_installer.py#L55

@flying-sheep
Copy link

Generally, creating them on installation is a good default. So if you want help @ewianda by allowing to disable it, that should be an option, not hardcoded for everyone.

@jvolkman
Copy link
Owner

Generally, creating them on installation is a good default. So if you want help @ewianda by allowing to disable it, that should be an option, not hardcoded for everyone.

The reason I'm thinking of disabling them completely is that the interpreter used to install wheels is not necessarily the interpreter selected for execution, and from what I understand, pyc files are specific to an interpreter version.

Pycross currently uses a fixed interpreter version when running its own tools, including the wheel installer.

@flying-sheep
Copy link

flying-sheep commented Apr 25, 2024

Yes, but distinguished by name, e.g. __pycache__/mymodule.cpython-39.pyc won’t stop python 3.12 from creating its own pyc/pyo files next to it.

The real problem is that apparently there’s some broken code snippet going around that can’t deal with those directories, i.e. openai/tiktoken#61 (comment)

It’s not your job to work around broken module discovery code. openai/tiktoken#152 is the correct fix here, I think this PR here can just be closed.

@rishabh-sagar-20
Copy link

Yes @flying-sheep, we should get this PR merged. I've been using a forked repository for so long; it's pretty hard to keep it updated.

@jvolkman
Copy link
Owner

It’s not your job to work around broken module discovery code. openai/tiktoken#152 is the correct fix here, I think this PR here can just be closed.

I agree with that. But this PR made me realize that generating pyc files at installation when using a fixed interpreter version for pycross tooling is likely not useful and unnecessarily expensive.

It used to be that the installer would run with the currently-selected rules_python interpreter - either the default, or the one chosen using the multi-version transitions. I changed to a fixed version so that install targets wouldn't need to re-execute every time someone switched between e.g. 3.10 and 3.11, if the package was compatible with both. But I neglected to consider the pyc file generation with that change.

It just so happens that pycross' tools are currently pinned to 3.12, the latest. But I wasn't planning on necessarily keeping that up-to-date. So when 3.13 ships, installed wheels will still just have 3.12 pyc files.

Maybe I should go back to installing with the selected interpreter, but it's definitely a trade-off.

@flying-sheep
Copy link

Sure, makes sense. Creating pyc files that go unused wastes 1–3 seconds or so.

@ewianda
Copy link
Contributor Author

ewianda commented Apr 26, 2024

@jvolkman I made the suggested change

@jvolkman jvolkman changed the title fix: Remove pycache directories after installation fix: Don't generate pyc files on installation Apr 26, 2024
@jvolkman jvolkman merged commit e7ed313 into jvolkman:main Apr 26, 2024
19 of 22 checks passed
@rishabh-sagar-20
Copy link

rishabh-sagar-20 commented Apr 28, 2024

When are you planning to release this @jvolkman? Thanks a lot for this 🙂

@jvolkman
Copy link
Owner

When are you planning to release this @jvolkman? Thanks a lot for this 🙂

@rishabh-sagar-20 this is released in 0.5.4.

@rishabh-sagar-20
Copy link

Thanks a lot @jvolkman 🙌🏻

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.

4 participants