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 SHOULD_GATE_ENTROPY condition #2668

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Sep 6, 2024

We can't turn it off if IS_TRACING because it needs to have the same setting when creating a snapshot as when using it.

@hoodmane hoodmane requested review from a team as code owners September 6, 2024 13:50
Copy link
Collaborator

@danlapid danlapid left a comment

Choose a reason for hiding this comment

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

LBTM but I don't really understand what's behind this change here or what it causes.
If you feel like you need a review here I'd suggest tagging someone from the python team.

@hoodmane hoodmane force-pushed the hoodmane/tracing-top-level-entropy branch from d125726 to 36d807f Compare September 6, 2024 13:58
@hoodmane hoodmane requested a review from dom96 September 6, 2024 13:59
@hoodmane
Copy link
Contributor Author

hoodmane commented Sep 6, 2024

Thanks @danlapid. I added Dominik who reported the bugs this fixes.

@dom96
Copy link
Collaborator

dom96 commented Sep 6, 2024

For context, while testing package memory snapshots I ran into two issues:

  • when tracing is enabled, which it is every time you use crossbow, the entropy code crashes the worker with "Cannot use random.seed() outside of request context"
  • invalidating caches was only done for baseline snapshots, so they weren't properly invalidated for package snapshots which lead to errors like "ModuleNotFoundError: No module named 'index'" (where index.py is the main module in the worker)

This PR fixes those. Thanks @hoodmane!

We can't turn it off if IS_TRACING because it needs to have the same setting
when creating a snapshot as when using it.
@hoodmane hoodmane force-pushed the hoodmane/tracing-top-level-entropy branch 2 times, most recently from 32f4395 to 250e0ae Compare September 10, 2024 11:54
Originally we had baseline snapshots shared and dedicated snapshots per worker.
But now we also have shared package snapshots. To be safe let's just always call
it.
@hoodmane hoodmane force-pushed the hoodmane/tracing-top-level-entropy branch from 250e0ae to 884486d Compare September 10, 2024 12:24
@hoodmane hoodmane merged commit 91b7770 into main Sep 10, 2024
13 checks passed
@hoodmane hoodmane deleted the hoodmane/tracing-top-level-entropy branch September 10, 2024 12:45
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