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

[Tune] Update generate_id algorithm #29900

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Conversation

bveeramani
Copy link
Member

Signed-off-by: Balaji [email protected]

Why are these changes needed?

tl;dr: uuid1 is based on time, so if you start trials at the same time, their IDs will conflict. See https://discuss.ray.io/t/clashing-trial-ids-run-id-in-wandbloggercallback/8046/3.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@krfricke
Copy link
Contributor

krfricke commented Nov 1, 2022

Should we catch duplicates and regenerate IDs if needed?

@krfricke
Copy link
Contributor

krfricke commented Nov 1, 2022

We've just discussed this in person.

Generally, keeping track of a set of unique IDs would be up to the generating function. The two places where we could run into duplicates is on initial trial creation and on Trial.reset() when we start a new trial from a previously errorred trial.

In both cases, tracking existing trial IDs in the experiment scope leads to a lot of added complexity (more so in the first case than in the latter). While we could refactor this, we believe that a switch to uuid4 will be enough: The likelihood that we end up with duplicates here is infinitesimal, and we haven't run into this problem in the past years before. So before we do this, let's see if uuid4 solves our problem.

@krfricke krfricke merged commit 5f84b7d into ray-project:master Nov 1, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
tl;dr: `uuid1` is based on time, so if you start trials at the same time, their IDs will conflict. See https://discuss.ray.io/t/clashing-trial-ids-run-id-in-wandbloggercallback/8046/3.

Signed-off-by: Weichen Xu <[email protected]>
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