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

Add archive experiment feature in backend #3359

Merged
merged 46 commits into from
Apr 15, 2020
Merged
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
492ee05
add new field in db schema and api schema
jingzhang36 Mar 24, 2020
e5ce3f7
auto genereted types for experiment storage state
jingzhang36 Mar 24, 2020
331b780
add archive and unarchive methods to backend for experiments.
jingzhang36 Mar 24, 2020
4509f15
auto generated archive/unarchive methods for epxeriments
jingzhang36 Mar 24, 2020
eb372ec
add archive and unarchive to client
jingzhang36 Mar 24, 2020
b86fb0a
set proper storage state when creating experiment
jingzhang36 Mar 24, 2020
eccc09a
retrieve storage state when we get/list epxeriment(s)
jingzhang36 Mar 24, 2020
1397b2b
change expection in test to have storage state
jingzhang36 Mar 24, 2020
f460bb8
add storage state in resource manager test
jingzhang36 Mar 24, 2020
7724c50
revise experiemnt server test
jingzhang36 Mar 24, 2020
de59fe5
revise api converter test
jingzhang36 Mar 24, 2020
8b6dc2b
integration test of experiment archive
jingzhang36 Mar 24, 2020
3ec8ccc
archive/unarchive experiment affect the storage state of runs in it
jingzhang36 Mar 24, 2020
1e82eaf
test all the runs in archive/unarchive experiment
jingzhang36 Mar 25, 2020
664b0e2
test all runs are archived/unarchived with their experiment in experi…
jingzhang36 Mar 25, 2020
7c47129
integration test
jingzhang36 Mar 25, 2020
68a42ec
integration test: value type mismatch in assertion
jingzhang36 Mar 25, 2020
3c82eea
unused import; default value for storage state
jingzhang36 Mar 25, 2020
5e5d476
autogen code for frontend
jingzhang36 Mar 25, 2020
2ddd451
reorder the fields in api experiment schema
jingzhang36 Mar 26, 2020
cfb8ea6
switch the position of the two enum to verify a hypothesis
jingzhang36 Mar 26, 2020
d5c7d65
Put a place hodler to prevent any valid item to take the value 0
jingzhang36 Mar 26, 2020
8473c4d
Get rid of the place holder since the cause of issue related to value…
jingzhang36 Mar 26, 2020
8a3a07e
The returned api experiment now has storage state field
jingzhang36 Mar 26, 2020
6851be1
create experiment return doesn't contain storege state
jingzhang36 Mar 26, 2020
9daaa03
Cleanup needs to clean runs and pipeliens now
jingzhang36 Mar 27, 2020
d0b4205
a missing client
jingzhang36 Mar 27, 2020
5cf6f1c
use resource reference as fileter instead of experiment uuid
jingzhang36 Mar 27, 2020
a3c7513
use same namespace in archive unit test
jingzhang36 Mar 27, 2020
d63dbbb
Leave archive/unarchive experiment integration test to a separate PR
jingzhang36 Mar 27, 2020
b79ba43
also need to update jobs when experiments are archived
jingzhang36 Apr 1, 2020
c0cb1b5
Merge remote-tracking branch 'origin/master' into arch_exp
jingzhang36 Apr 1, 2020
c86dc48
Change of unarchiving logic. When experiment is unarchived, jobs/runs in
jingzhang36 Apr 1, 2020
0676ddf
add unit test for the job status in archived/unarchived experiment
jingzhang36 Apr 1, 2020
8d8516d
change archive state to 3 value enum; add experiment integration test
jingzhang36 Apr 2, 2020
4ba3621
make archive state 3 value enum to avoid 0 value mapped to available;…
jingzhang36 Apr 2, 2020
a74bac0
run swagger autogen
jingzhang36 Apr 2, 2020
4888ea7
fix an expected value
jingzhang36 Apr 2, 2020
16d59be
Merge branch 'arch_exp' of github.com:jingzhang36/pipelines into arch…
jingzhang36 Apr 2, 2020
faba821
fix experiment server test
jingzhang36 Apr 2, 2020
9b8fadb
add job check in experiment server test
jingzhang36 Apr 2, 2020
13260b0
update job crds
jingzhang36 Apr 6, 2020
e796869
Merge remote-tracking branch 'upstream/master' into arch_exp
jingzhang36 Apr 7, 2020
018d8d9
fix a typo
jingzhang36 Apr 8, 2020
934c7f9
Merge remote-tracking branch 'origin/master' into arch_exp
jingzhang36 Apr 8, 2020
ed821d0
remove accidentally included irrelevant changes
jingzhang36 Apr 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
change archive state to 3 value enum; add experiment integration test
jingzhang36 committed Apr 2, 2020
commit 8d8516dbe43f553d61b69a9d3f57dda858a1972f
22 changes: 20 additions & 2 deletions backend/src/apiserver/storage/experiment_store.go
Original file line number Diff line number Diff line change
@@ -252,6 +252,18 @@ func (s *ExperimentStore) ArchiveExperiment(expId string) error {
"Failed to create query to archive experiment %s. error: '%v'", expId, err.Error())
}

updateRunsWithExperimentUUIDSql, updateRunsWithExperimentUUIDArgs, err := sq.
Update("run_details").
SetMap(sq.Eq{
"StorageState": api.Run_STORAGESTATE_ARCHIVED.String(),
}).
Where(sq.Eq{"ExperimentUUID": expId}).
ToSql()
if err != nil {
return util.NewInternalServerError(err,
"Failed to create query to archive the runs in an experiment %s. error: '%v'", expId, err.Error())
}

// TODO(jingzhang36): use inner join to replace nested query for better performance.
filteredRunsSql, filteredRunsArgs, err := sq.Select("ResourceUUID").
From("resource_references as rf").
@@ -263,7 +275,6 @@ func (s *ExperimentStore) ArchiveExperiment(expId string) error {
return util.NewInternalServerError(err,
"Failed to create query to filter the runs in an experiment %s. error: '%v'", expId, err.Error())
}

updateRunsSql, updateRunsArgs, err := sq.
Update("run_details").
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do anything for jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this!
There is a little bit complexity here. We can sync quickly to be on the same page.
First of all, Job doesn't have the status of being "archived". But it does have the status of being "enabled". Then, we have a couple of options
(1) kind of abuse the "enabled" status in Job to simulate "unarchived" status in Run and Experiment, i.e., "enabled=true" means "unarchived" and "enabled=false" means "archived"
(2) add "archived" status for Job, but that would make the status of Job too complicated since it has two control factors now.

I will prefer (1) but want to check there will not introduce side effect. E.g. "enable" in Job has some existing specific meaning or treatment in KFP.

Copy link
Member

Choose a reason for hiding this comment

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

(1) sounds good for me. I can't think of any side effect. @IronPan, anything catching your mind?

When an experiment is archived, I feel like disabling the job is the right action to take, otherwise the job is still scheduled to run, right?
On the other side, when an experiment is unarchived, I think it's probably fine that we don't re-enable jobs that were previously disabled because of the experiment archiving. Because that requires extra complexity to keep track of why an job got disabled, which I don't think worth the effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with (1) and @chensun's arguments.

The behavior can be illustrated in a different way, we are not reusing enabled field as archived, but rather when we archive an experiment, it will automatically disable all jobs in that experiment. (very reasonable default behavior) Therefore, when we unarchive, it shouldn't enable all jobs.

Whether we need an separate archive field depends on if there are use-cases to archive an individual job. Sounds to me users can just delete jobs they don't want and disable jobs they want temporarily disabled. There's no need for an archive option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used "enabled" field for job

SetMap(sq.Eq{
@@ -314,11 +325,18 @@ func (s *ExperimentStore) ArchiveExperiment(expId string) error {
"Failed to archive experiment %s. error: '%v'", expId, err.Error())
}

_, err = tx.Exec(updateRunsWithExperimentUUIDSql, updateRunsWithExperimentUUIDArgs)
if err != nil {
tx.Rollback()
return util.NewInternalServerError(err,
"Failed to archive runs with ExperimentUUID being %s. error: '%v'", expId, err.Error())
}

_, err = tx.Exec(updateRunsSql, updateRunsArgs...)
if err != nil {
tx.Rollback()
return util.NewInternalServerError(err,
"Failed to archive all runs in an experiment %s. error: '%v'", expId, err.Error())
"Failed to archive runs with experiment reference being %s. error: '%v'", expId, err.Error())
}

_, err = tx.Exec(updateJobsSql, updateJobsArgs...)