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

chore: fix random/grid searcher bug with max_concurrent_trials #4836

Merged
merged 2 commits into from
Aug 23, 2022
Merged

chore: fix random/grid searcher bug with max_concurrent_trials #4836

merged 2 commits into from
Aug 23, 2022

Conversation

stoksc
Copy link
Contributor

@stoksc stoksc commented Aug 22, 2022

Description

This change fixes a bug where, if trials exited early under the grid or random searchers when using max_concurrent_trials, it would cause double the amount of trials to be spawned in its place, exceeding max concurrent trials. This was caused by the searcher hooking into both 'trial exited early' and 'trial closed' events as notification a trial had closed when 'trial closed' is already emitted for every trial ('trial exited early' is just an additive event for failed and invalid HP trials).

Test Plan

  • add an e2e (this is an e2e and not a searcher unit because this was an experiment<->trial<->searcher interaction bug)

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

@stoksc stoksc requested a review from a team as a code owner August 22, 2022 14:56
@cla-bot cla-bot bot added the cla-signed label Aug 22, 2022
@netlify
Copy link

netlify bot commented Aug 22, 2022

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 081129a
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/63053cb8ad63110008a3264c

@stoksc stoksc requested a review from ioga August 22, 2022 15:00
@stoksc stoksc requested a review from liamcli August 22, 2022 15:00
@rb-determined-ai
Copy link
Member

rb-determined-ai commented Aug 23, 2022

force-merging since stokc is out at dinner and it needs to land for the release.

(also the only failures are an unrelated lint failure)

@rb-determined-ai rb-determined-ai merged commit ec7c8e3 into determined-ai:master Aug 23, 2022
@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.2 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants