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: fetch experiment in case config data is not contained #8789

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

keita-determined
Copy link
Contributor

Description

There is a bug after #8765
The context menu (right click menu) in the new experiment table has Hyperparameter Search option. When the option is clicked on the new experiment table view, the data from experiment-search API does not contain config data since it is deprecated in #8732 and unlinked on web in #8765. Therefore, we need to fetch experiment data through getExperiment API for the missing config.
Before this fix, Unexpected Error shows up after clicking context menu.

Test Plan

  • Check if Hyperparapeter serach modal shows data and works as before by clicking context menu in the new experiment table, and Hyperparapeter Search button in experiment detail page

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.

Ticket

Copy link

netlify bot commented Feb 2, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit fb0362d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65bd8449a96e970008d266ad
😎 Deploy Preview https://deploy-preview-8789--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@keita-determined keita-determined force-pushed the fix/null-check branch 2 times, most recently from 8e4040e to 95a8f70 Compare February 2, 2024 02:58
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (f1a45ae) 53.00% compared to head (fb0362d) 42.71%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #8789       +/-   ##
===========================================
- Coverage   53.00%   42.71%   -10.29%     
===========================================
  Files         633      732       +99     
  Lines       72314   128610    +56296     
  Branches        0     2241     +2241     
===========================================
+ Hits        38331    54942    +16611     
- Misses      33983    73510    +39527     
- Partials        0      158      +158     
Flag Coverage Δ
harness ?
web 42.52% <7.40%> (?)

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

Files Coverage Δ
webui/react/src/services/decoder.ts 23.46% <0.00%> (ø)
.../react/src/components/ExperimentActionDropdown.tsx 23.18% <7.54%> (ø)

... and 701 files with indirect coverage changes

@keita-determined keita-determined marked this pull request as ready for review February 2, 2024 03:37
@keita-determined keita-determined requested a review from a team as a code owner February 2, 2024 03:37
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

given that we use this modal in the details view (where we should have the full config available), could we instead show a loader modal in the context menu that then shows the hyperparameter search modal once the experiment is fetched?

sorry, to reword, perhaps instead have the context menus in the experiment lists show a modal that just consists of a loading spinner that loads the full experiment, then passes it on to the hyperparameter search modal.

@keita-determined keita-determined force-pushed the fix/null-check branch 2 times, most recently from 42b12b3 to 00edc6c Compare February 3, 2024 00:08
@keita-determined
Copy link
Contributor Author

fixed

@keita-determined keita-determined merged commit c5afb6c into main Feb 6, 2024
74 of 86 checks passed
@keita-determined keita-determined deleted the fix/null-check branch February 6, 2024 19:26
dai-release bot pushed a commit that referenced this pull request Feb 16, 2024
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
* fix: fetch experiment in case config data is not contained

* test: fix test cases

* fix: minor fix

* fix: get experiment in top level
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.

2 participants