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

Projects
None yet
3 participants
@rileyjbauer
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

/test kubeflow-pipeline-e2e-test

2 similar comments
@rileyjbauer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

/test kubeflow-pipeline-e2e-test

@rileyjbauer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

/test kubeflow-pipeline-e2e-test

@rileyjbauer rileyjbauer force-pushed the rileyjbauer:allow-creating-runs-from-anywhere branch 3 times, most recently from bc6072c to f8fd216 Apr 17, 2019

@rileyjbauer rileyjbauer force-pushed the rileyjbauer:allow-creating-runs-from-anywhere branch from 6e9dd6d to 52365d8 Apr 18, 2019

@rileyjbauer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/test kubeflow-pipeline-e2e-test

@rileyjbauer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/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;

This comment has been minimized.

Copy link
@yebrahim

yebrahim Apr 18, 2019

Contributor

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.

This comment has been minimized.

Copy link
@rileyjbauer

rileyjbauer Apr 19, 2019

Author Contributor

Agreed. Added a TODO

}, waitTimeout);
});

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

This comment has been minimized.

Copy link
@yebrahim

yebrahim Apr 18, 2019

Contributor

extra "experiment"

This comment has been minimized.

Copy link
@rileyjbauer

rileyjbauer Apr 19, 2019

Author Contributor

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);

This comment has been minimized.

Copy link
@yebrahim

yebrahim Apr 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@rileyjbauer

rileyjbauer Apr 19, 2019

Author Contributor

Nope. Experiments can exist without runs

@yebrahim

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

/lgtm overall, just a few comments.

rileyjbauer added some commits Apr 19, 2019

@rileyjbauer rileyjbauer force-pushed the rileyjbauer:allow-creating-runs-from-anywhere branch from 5bf7a86 to 79cc1be Apr 19, 2019

@rileyjbauer rileyjbauer force-pushed the rileyjbauer:allow-creating-runs-from-anywhere branch from 3f0d243 to 70c67dd Apr 19, 2019

Remove create run without experiment integration test for now as it f…
…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

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 22, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Apr 22, 2019

[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

5 checks passed

Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
coverage/coveralls Coverage remained the same at 92.837%
Details
kubeflow-pipeline-e2e-test Job succeeded.
Details
tide In merge pool.
Details

@rileyjbauer rileyjbauer deleted the rileyjbauer:allow-creating-runs-from-anywhere branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.