feat(frontend): Show Archive tab for runs in Experiment Detail page #5040
Conversation
|
/test kubeflow-pipeline-e2e-test |
|
The changes seem to break frontend integration testing, you'll need to update https://github.com/kubeflow/pipelines/blob/master/test/frontend-integration-test/helloworld.spec.js accordingly. You can find the error log at https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/kubeflow_pipelines/5040/kubeflow-pipeline-e2e-test/1354907745475629056#1:build-log.txt%3A5305 |
Thank you for pointing it out! I have applied the fix for E2E tests |
|
@zijianjoy Thank you for building up the new UX! It's absolutely great! I've just got some minor code organization questions, did you investigate if we can refactor the AllRunsAndArchive.tsx, so that it fits the new needs by As far I can tell, the two components are almost identical in visual appearance, while the only thing that's different is -- that RunListsRouter: |
|
/cc @StefanoFioravanzo |
|
@Bobgy absolutely! I will take a look at the PR in a day or two |
That is a good point, I have explored merging Therefore I suggest to start a new file called How do you think? |
|
Thanks, sounds like a good plan. |
| */ | ||
| class RunListsRouter extends React.PureComponent<RunListsRouterProps> { | ||
| protected _isMounted = true; | ||
| private _runlistRef = React.createRef<RunList>(); |
There was a problem hiding this comment.
Writing some longer term issues we need to address later. (Non blocker for this PR)
https://reactjs.org/docs/refs-and-the-dom.html
It's recommended to avoid using ref when possible, because the data/event flow mechanism conflicts with React's recommendation of one-way data flow philosophy (see https://reactjs.org/docs/thinking-in-react.html).
For example, instead of exposing open() and close() methods on a Dialog component, pass an isOpen prop to it.
Similar to this example, I think to avoid exposing refresh() as a public method, we could have a prop called sth like refreshCounter: number, the child components can do a refresh whenever it sees an update in refreshCounter -- using componentDidUpdate or useEffect react hook. In this way, we no longer need to add refs all the way and trigger a method to refresh components, one refresh would be as simple as a state update to the refreshCounter.
We'd also need a onLoad callback, that will be triggered by child components when a page finishes loading.
I think these comments may be hard to follow, we can follow up later to dive into more details. I'd like to start these discussions now, because I know there are many anti-patterns in the existing code and I hope they do not keep contaminating new code written when possible (in this case, it's fairly hard to avoid the existing pattern).
After avoiding the bad patterns, we can write components using functions and react hooks: https://reactjs.org/docs/hooks-overview.html. It's a nice way to make abstraction on state changes (and a lot more!).
There was a problem hiding this comment.
@Bobgy this is a great argument! Do you think you could start an issue to document these antipatterns, maybe with a brief proposed initial solution, so we can keep track of them? You know the repository and the codebase better than anyone else. If we can keep our context there and start a discussion, then we will be able to address these one by one
There was a problem hiding this comment.
Thank you @Bobgy and @StefanoFioravanzo ! I agree with following one-way data flow approach and we should avoid using Ref as much as we can. I have attempted to change my PR to follow the suggested guide. But I found out that it requires changes beyond just ExperimentDetail.tsx, therefore I cautiously replace the implementation in ExperimentDetail.tsx with the one-way data flow practice, while keeping other files with Ref approach for now. RunList.tsx change affects multiple pages so I leave it unchanged to avoid large PR. We can take it as a first step of migration away from Ref, and gradually refactoring the codebase along the way.
To @StefanoFioravanzo: Good suggestion! I have created #5118 so feel free to make your proposal there. The goal is to document the agreed practices in README file so all contributors can follow along.
There was a problem hiding this comment.
Awesome, I'll put more there soon
|
@zijianjoy Why did you rename |
|
BTW @zijianjoy This contribution is great! I think @Bobgy made a good pass on the code and I don't have much to add for now. I would like just to confirm this, design wise: KFP currently treats archival of Experiments and Runs completely independently, correct? I can have an archived Experiments with just active Runs and vice-versa. If this holds, I would propose to make the state of the Experiment super explicit, in the |
|
To clarify, restoring runs after an experiment is archived should not succeed. But agree that making it visually distinguishable for experiment state is helpful! That's a good suggestion. It can be a separate PR or not. You can make the decision |
Actually I am doing the opposite, I don't know why the codebase has such a name |
I have created #5114 to discuss the relationship between Experiment and Runs. Once we made a decision, we can create subsequent PR for the change. Currently I agree that Runs in Archived Experiment cannot be restored, but our backend doesn't enforce it. We can make changes based on the discussion result. |
|
/lgtm Awesome, this is a great first step towards more declarative implementation! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, zijianjoy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, zijianjoy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of your changes:
Video: https://youtu.be/wlE0rd6235s
Behavior: When user selects an Experiment, it will shows a tab selection in Experiment Detail page for
ActiveandArchivedruns.Archivebutton.Restorebutton.Checklist:
Do you want this pull request (PR) cherry-picked into the current release branch?
Learn more about cherry-picking updates into the release branch.