Skip to content

Enable secret seeds#228

Merged
ngc92 merged 2 commits into
mainfrom
ngc92/secret-seeds
Apr 3, 2025
Merged

Enable secret seeds#228
ngc92 merged 2 commits into
mainfrom
ngc92/secret-seeds

Conversation

@ngc92
Copy link
Copy Markdown
Collaborator

@ngc92 ngc92 commented Mar 29, 2025

Enable (and use) secret seeds in the default python runner (examples/eval.py).
Secret seeds are not used in eval.cu yet.

Requires a modal redeploy.
Because eval.py is baked into the task definitions, none of the existing tasks will benefit from this. We'll need to figure out how to update existing tasks separately.

@ngc92 ngc92 requested a review from S1ro1 March 29, 2025 22:09
@ngc92 ngc92 force-pushed the ngc92/secret-seeds branch from 21adeed to 7e5943c Compare March 29, 2025 22:17
@ngc92 ngc92 force-pushed the ngc92/secret-seeds branch from 7e5943c to 880414c Compare March 29, 2025 23:02
@ngc92 ngc92 mentioned this pull request Mar 30, 2025
Copy link
Copy Markdown
Member

@S1ro1 S1ro1 left a comment

Choose a reason for hiding this comment

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

Except of this, LGTM!

Comment thread examples/eval.py

mode = sys.argv[1]
tests = get_test_cases(sys.argv[2])
seed = os.getenv("POPCORN_SEED")
Copy link
Copy Markdown
Member

@S1ro1 S1ro1 Apr 3, 2025

Choose a reason for hiding this comment

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

A small nit (have highligted just one line, but should replace 2 below as well):

Suggested change
seed = os.getenv("POPCORN_SEED")
seed = int(os.getenv("POPCORN_SEED", 42))
set_seed(seed)
tests = get_test_cases(sys.argv[2], seed)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

not sure about these changes:

  1. I really dislike set_seed, it adjusts global state, and if you were to actually use it you couldn't run a subset of the tests; you would have to run them all, and in the same order.
  2. I explicitly want to have the None value here, which tells get_test_cases to use the seeds from the yml file as-is.

We could put an unconditional set_seed(42) call here, as an insurance policy in case a test unintentionally relies on a source of randomness not controlled by the explcitly-passed seed argument.

Maybe we should actually add a new bot commend "check-task", that runs the test case generators in randomized order an checks that they're reproducible?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh, makes sense, anyway it's approved so merge anytime @ngc92. Probably let's do it before #230 as one will need to update migrations to match the previous one.

@ngc92 ngc92 merged commit 7d7a96e into main Apr 3, 2025
@ngc92 ngc92 deleted the ngc92/secret-seeds branch April 15, 2025 13:49
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