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

Allow creating runs without experiments #1175

Merged

Conversation

rileyjbauer
Copy link
Contributor

@rileyjbauer rileyjbauer commented Apr 16, 2019

Following #1089, this PR adds Create run button to Experiment/All run list pages.

Also updates the frontend integration test to include creating a run without selecting an experiment, and filtering the experiment list.


This change is Reviewable

@rileyjbauer
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

2 similar comments
@rileyjbauer
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@rileyjbauer
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@rileyjbauer rileyjbauer force-pushed the allow-creating-runs-from-anywhere branch 3 times, most recently from bc6072c to f8fd216 Compare April 17, 2019 23:11
@rileyjbauer rileyjbauer force-pushed the allow-creating-runs-from-anywhere branch from 6e9dd6d to 52365d8 Compare April 18, 2019 07:27
@rileyjbauer
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@rileyjbauer
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@@ -77,11 +78,11 @@ class AllRunsList extends Page<{}, AllRunsListState> {
private _selectionChanged(selectedIds: string[]): void {
const toolbarActions = [...this.props.toolbarProps.actions];
// Compare runs button
toolbarActions[1].disabled = selectedIds.length <= 1 || selectedIds.length > 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

The design of this toolbarActions array is a source of potential problems, I kind of regret doing it this way. I'd recommend adding a TODO to refactor it into an object with tighter type semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Added a TODO

}, waitTimeout);
});

it('displays both custom and default experiment experiment list page', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

extra "experiment"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

it('displays both custom and default experiment experiment list page', () => {
$('.tableRow').waitForVisible();
const rows = $$('.tableRow').length;
assert(rows === 2, 'there should now be two experiments in the table, instead there are: ' + rows);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, does the default experiment disappear if this experiment-less run goes away? What if it's archived for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Experiments can exist without runs

@yebrahim
Copy link
Contributor

/lgtm overall, just a few comments.

@rileyjbauer rileyjbauer force-pushed the allow-creating-runs-from-anywhere branch from 5bf7a86 to 79cc1be Compare April 19, 2019 07:45
@rileyjbauer rileyjbauer force-pushed the allow-creating-runs-from-anywhere branch from 3f0d243 to 70c67dd Compare April 19, 2019 18:23
…ails due to the default experiment being deleted at the end of the API initialization and integration test suites
@rileyjbauer rileyjbauer changed the title [WIP] - Allow creating runs without experiments Allow creating runs without experiments Apr 19, 2019
@yebrahim
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yebrahim

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit b292663 into kubeflow:master Apr 22, 2019
@rileyjbauer rileyjbauer deleted the allow-creating-runs-from-anywhere branch May 6, 2019 22:14
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.

3 participants