Clear default exp table on delete and create default exp on run create if none exists #1199
Conversation
if no default exists With this change, if the delete experiment API is called on the default experiment, then the ID will also be removed from the default_experiments table. Additionally, if the default experiment doesn't exist and a new run is created without an experiment, a new default experiment will be created, and the run will be placed within this experiment.
rileyjbauer
changed the title
…ing and deleting default experiment
rileyjbauer
force-pushed
the
create-and-delete-default-exp
branch
from
3445425
to
57fc0f0
Compare
| @@ -188,6 +189,11 @@ func (s *ExperimentStore) DeleteExperiment(id string) error { | |||
| tx.Rollback() | |||
| return util.NewInternalServerError(err, "Failed to delete experiment %s from table", id) | |||
| } | |||
| err = s.defaultExperimentStore.ClearDefaultExperimentId(tx, id) | |||
There was a problem hiding this comment.
how about naming this wipeoutExperimentIdIfDefault or something equvalent.
with this name it's a bit hard to follow without context
There was a problem hiding this comment.
Yeah, you're very right. How about UnsetDefaultExperimentIdIfIdMatches ?
|
/lgtm |
|
/test kubeflow-pipeline-e2e-test |
|
Why allow the default experiment to be deleted? If it's created automatically on init, I'd think it's a permanent resource. |
"Permanence" can be difficult to ensure in practice, and manually specifying a UUID is generally discouraged as well. The |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan 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: IronPan 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 |
With this change, if the delete experiment API is called on the default
experiment, then the ID will also be removed from the default_experiments
table.
Additionally, if the default experiment doesn't exist and a new run is
created without an experiment, a new default experiment will be created,
and the run will be placed within this experiment.
This PR also fixes an issue in some of the backend store files where the connection to the database was not being closed. This wasn't much of an issue in production, but it appeared that in the test environment, only one connection was allowed, so failing to close the connection would cause subsequent queries to fail, claiming that the table no longer existed.
This change is