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.

Sign up for GitHub

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

Jump to bottom

chore: refactor searcher operations out of master side searchers #10024

Merged
merged 4 commits into from
Oct 25, 2024
Merged

chore: refactor searcher operations out of master side searchers #10024

merged 4 commits into from
Oct 25, 2024

Conversation

azhou-determined
Copy link
Contributor

@azhou-determined azhou-determined commented Oct 4, 2024 •
edited by jira bot
Loading

Ticket

Description

refactor of master-side searchers:

  • remove usage of searcher operations
  • remove usage of max_length master-side
  • rewrite of preview-search
  • renames trial -> run in new code
  • rewrite of progress calculation

TODO:

  • code cleanup / add comments
  • rewrite/add tests
  • update docs

Test Plan

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@azhou-determined azhou-determined requested review from a team as code owners October 4, 2024 20:38
@cla-bot cla-bot bot added the cla-signed label Oct 4, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.86%. Comparing base (db8d4b0) to head (ff5a7ad).

❗ There is a different number of reports uploaded between BASE (db8d4b0) and HEAD (ff5a7ad). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (db8d4b0) HEAD (ff5a7ad)
harness 6 1
web 1 0
Additional details and impacted files 
@@                      Coverage Diff                      @@
##           searcher-context-removal   #10024       +/-   ##
=============================================================
- Coverage                     58.04%   39.86%   -18.18%     
=============================================================
  Files                           747      117      -630     
  Lines                        103306    10048    -93258     
  Branches                       3617        0     -3617     
=============================================================
- Hits                          59959     4006    -55953     
+ Misses                        43214     6042    -37172     
+ Partials                        133        0      -133
Flag Coverage Δ
harness 38.94% <ø> (-31.63%) ⬇️
web ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 694 files with indirect coverage changes

@determined-ci determined-ci requested a review from a team October 15, 2024 17:12
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Oct 15, 2024
@determined-ci determined-ci requested a review from a team October 16, 2024 15:30
@azhou-determined azhou-determined force-pushed the searcher-refactor branch 3 times, most recently from 9e6084c to 835fc8e Compare October 24, 2024 01:54
@azhou-determined azhou-determined requested a review from a team as a code owner October 24, 2024 19:24
stoksc
stoksc reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

lgtm, just a few questions but once we go over them i dont feel the need to review again.

master/internal/api_trials.go Show resolved Hide resolved
master/internal/db/postgres_trial.go Outdated Show resolved Hide resolved
master/internal/db/postgres_trial.go Outdated Show resolved Hide resolved
master/pkg/searcher/tournament.go Outdated Show resolved Hide resolved
master/pkg/searcher/simulate.go Show resolved Hide resolved
master/internal/trial.go Outdated Show resolved Hide resolved
master/pkg/searcher/actions.go Outdated Show resolved Hide resolved
master/internal/experiment.go Show resolved Hide resolved
@rb-determined-ai rb-determined-ai force-pushed the searcher-context-removal branch 2 times, most recently from dfd7d50 to db8d4b0 Compare October 24, 2024 22:06
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

2 similar comments
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

stoksc
stoksc approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

lgtm pending that last name change for closed/stopped

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@azhou-determined azhou-determined force-pushed the searcher-refactor branch 2 times, most recently from 0d28c2e to a0d898a Compare October 25, 2024 00:07
@azhou-determined azhou-determined merged commit 2970bb2 into searcher-context-removal Oct 25, 2024
78 of 92 checks passed
@azhou-determined azhou-determined deleted the searcher-refactor branch October 25, 2024 15:48
rb-determined-ai pushed a commit that referenced this pull request Oct 25, 2024
@azhou-determined @rb-determined-ai
chore: refactor searcher operations out of master side searchers (#10024
544c404 
)

Refactors searchers in master to no longer issue operation workloads to trials. This only functionally affects ASHA search algorithms, which are now preemption-based instead of promotion-based.

- Removes all `SearcherOperation` APIs.
- Removes the non-stopping variant of adaptive ASHA, deprecating the `stop_once` config option.
- Removes and deprecates the `max_length` requirement for all experiments. 
- Introduces `time_metric` and `max_time` as new resource configuration options for ASHA searchers.
- Reporting validation metrics now reports to the searcher (only affects ASHA searchers).
- Preview HP Search was refactored to estimate training lengths based on ASHA rungs.
@azhou-determined azhou-determined mentioned this pull request Oct 25, 2024
Merged
5 tasks
rb-determined-ai pushed a commit that referenced this pull request Oct 25, 2024
@azhou-determined @rb-determined-ai
chore: refactor searcher operations out of master side searchers (#10024
78a58d6 
)

Refactors searchers in master to no longer issue operation workloads to trials. This only functionally affects ASHA search algorithms, which are now preemption-based instead of promotion-based.

- Removes all `SearcherOperation` APIs.
- Removes the non-stopping variant of adaptive ASHA, deprecating the `stop_once` config option.
- Removes and deprecates the `max_length` requirement for all experiments. 
- Introduces `time_metric` and `max_time` as new resource configuration options for ASHA searchers.
- Reporting validation metrics now reports to the searcher (only affects ASHA searchers).
- Preview HP Search was refactored to estimate training lengths based on ASHA rungs.
azhou-determined added a commit that referenced this pull request Oct 25, 2024
@azhou-determined
chore: refactor searcher operations out of master side searchers (#10024
17e942a 
)

Refactors searchers in master to no longer issue operation workloads to trials. This only functionally affects ASHA search algorithms, which are now preemption-based instead of promotion-based.

- Removes all `SearcherOperation` APIs.
- Removes the non-stopping variant of adaptive ASHA, deprecating the `stop_once` config option.
- Removes and deprecates the `max_length` requirement for all experiments.
- Introduces `time_metric` and `max_time` as new resource configuration options for ASHA searchers.
- Reporting validation metrics now reports to the searcher (only affects ASHA searchers).
- Preview HP Search was refactored to estimate training lengths based on ASHA rungs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rb-determined-ai rb-determined-ai rb-determined-ai left review comments

@stoksc stoksc stoksc approved these changes

@molinamelendezj molinamelendezj Awaiting requested review from molinamelendezj molinamelendezj was automatically assigned from determined-ai/infrastructure

Assignees
No one assigned
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@azhou-determined @determined-ci @thiagodallacqua-hpe @stoksc @rb-determined-ai