-
Notifications
You must be signed in to change notification settings - Fork 356
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
feat: Update the "Continue Single trial experiment" workflow #8526
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can consolidate the Continue trial modal into the current Fork modal?
Some user experience comments:
When reactivating experiment, should we limit the parameters user can change in full config? For example, I can change workspace and project in full config, but the reactivated experiment should still be at the current project.
Should we allow continue/reactivate function when the experiment is running?
))} | ||
{options?.slice(0, 3).map((option) => | ||
option?.content ? ( | ||
option.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit hacky in my opinion, do you think there might be a way around it? Such as options?: (ButtonOption | DropdownOption)[]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gt2345 I could do that, I would then have to update several codepaths to handle either type which also felt a but hacky.
To me it seems a little weird to force us to only show a "Button" or "Dropdown" here. It seems possible that one day we might want to show an icon, or freeform text, or some other component here. I can do this but this seemed like a more future-proof approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gt2345 I made several updates to this. We can now show any react node, and show multiple actions per react node, the latter was needed to show both of the actions in the button dropdown on the overflow list.
@gt2345 I updated the UI to show an error if the Workspace or Project is changed |
@gt2345 for the last point I have updated this for only terminal states. |
I actually started this work doing that at first. Ultimately, I ended up making a separate modal and just moved any shared functions to a util file. There is enough difference in the modals that I think trying to fit all of the differing functionality in one modal would result in a single overly complex component. |
Thanks for being open to it, but I think it might be best if we loop into backend to decide the full list of properties that cannot be modified with this continue action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy path works as expected, the thing I'm not sure is that could there be other config parameters that cannot be modified for continue
action, and do we care that if the submitted config is inconsistent with the actual config?
</React.Suspense> | ||
)} | ||
{!isReactivate && !modalIsInAdvancedMode && ( | ||
<Body>Start a new experiment from the current trial’s latest checkpoint.</Body> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a grayish text color if possible according to design file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically yes we should have some validation (around what is a valid config) but I think we're ok to leave feature parity with the current continue feature since that is broader scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hkumar92 for clarification.
@julian-determined-ai Can we still address the style if possible?
I cannot recreate this, can you give me steps to reproduce the issue> CD429A40-5613-4B2A-AAD6-EF70E663E912.mov |
Interesting, I don't see anything we are doing differently... Also I suppose it's okay to remove this functionality based on what Haran said? Screen.Recording.2023-12-07.at.12.17.29.PM.mov |
8e2b9a2
to
d5ce13c
Compare
Overall looks great, notices one unusual behavior. When the modal opens for the first time, the button should be disabled but not (which if fine), but when navigate to full config then navigate back, the additional batch field is auto filled. Is this by design? Screen.Recording.2023-12-12.at.11.43.12.AM.mov |
@gt2345 this is actually by design and as intended. I agree that the behavior is a bit awkward. I think re-visiting this when/if we decide to change how we validate changes in the full config makes sense though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Implements the "Continue Trial" workflow as described in WEB-1857
Test Plan
Commentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket
WEB-1857