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

Set 'deleted' to True when purging a history dataset association #3572

Merged
merged 2 commits into from Feb 8, 2017

Conversation

Projects
None yet
6 participants
@nsoranzo
Copy link
Member

commented Feb 7, 2017

Fix #3548 , bug introduced in commit 58f677a.

Add API and unit test cases.
Add migration to fix misaligned databases.

Set 'deleted' to True when purging a history dataset association
Fix #3548

Add API and unit test cases.
Add migration to fix misaligned databases.
Sync API history purge with hda implementation
Add API and unit test cases.
Import order and Python3 fixes.
@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

I've added a second commit which adds test cases also for histories (not affected by the issue) and avoids deleting them twice when purging.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

I don't know what the deal is with this hung test - I've seen it occasionally for other tests also but there doesn't seem to be a Jenkins record of it. At any rate I am confident the tests wouldn't fail as a result of this PR.

@natefoo @blankenberg do either of you have a good reason why some purged datasets aren't deleted - I feel like I may have heard a reason at one point from one of you why but my memory is faulty and it is really counter intuitive now why this would be an allowed state.

@dannon

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

+1, thanks @nsoranzo.

@dannon dannon merged commit ed3062b into galaxyproject:dev Feb 8, 2017

4 of 5 checks passed

integration test Test scheduled.
Details
api test Build finished. 263 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 138 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details

@nsoranzo nsoranzo deleted the nsoranzo:fix_3548 branch Feb 8, 2017

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Yes, the reason was that if you look at a purged history, you still want to see the history in the "state" it was in when purged. If you delete the purged HDAs, you lose the record of which datasets were deleted and which were active at the time you deleted and/or purged the history.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

To add to that, I don't know if the UI allows for extracting a workflow from purged histories, but it could/should, and you would definitely want the correct HDA deleted state in that case.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

So @natefoo do you think we should back out of this PR then?

@dannon

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Hrmm. Are we sure there's actually value in seeing deleted vs purged for purged histories? I see that we've lost information here, for sure, in aligning the flags. I just am skeptical it's info worth keeping.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

It's my opinion that it should be backed out, yes.

@dannon

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Guess we should treat this like a -1, then. I've opened a PR to revert it. We'll want to follow up on the original bug(s) mentioned by @nsoranzo in the previous issue and see if there's something else going on.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2017

@natefoo That was not the case pre-release_15.03, I doubt that was changed on purpose to allow this scenario.

@martenson

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

-1 and +1 are explicit - I wouldn't 'treat' things like something that is explicitly specified

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Agreed, I'd prefer more discussion before throwing an explicit -1 on this.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@nsoranzo It was when I added hda.purged - I am not sure when it broke, however.

@dannon

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

I'm not stopping anyone talking about things at all, @natefoo or @martenson, I merely opened a PR to revert it reflecting the dissent expressed here, since I'm the one that merged this. Keep on talking.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2017

For what I see, it is not possible in 17.01 (so before this PR) to extract a workflow from a purged history (in the UI at least) because you cannot set it as current history.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

I'm -.5 on backing out - I prefer the new behavior. I understand information was lost but that wasn't t an intuitive way to capture it and it isn't worth the confusion and it has broken for sometime anyway.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@nsoranzo Ah okay, I misunderstood what you were referring to when you said it was previously possible.

I really think it's less useful to view a purged history and see nothing - and have no idea what you did - than it is to see it look the way it was when you deleted it. If you (via bioblend) are treating undeleted-but-purged HDAs in a purged history as anything but purged that seems to be a usage error?

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2017

@natefoo When I said:

That was not the case pre-release_15.03, I doubt that was changed on purpose to allow this scenario.

I meant that in release_15.01, when you purged a dataset through the API, it was also deleted: https://github.com/galaxyproject/galaxy/blob/release_15.01/lib/galaxy/webapps/galaxy/api/history_contents.py#L490

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Ah, ok. That behavior would have been different from the UI behavior at the time, however.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

A bit of clarification, in the UI, there is no way to purge an undeleted HDA other than purging the history, or via the cleanup scripts. So if you purged an individual HDA, it would've been already explicitly deleted.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2017

in the UI, there is no way to purge an undeleted HDA other than purging the history

That's correct, in fact my original bug report #3548 was that purging a dataset through the API did not mark it as deleted.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

Sorry for the run around on this @nsoranzo. So in that case, it makes sense to mark deleted when purging an individual HDA via the API assuming current methods for purging histories presumably do not individually purge HDAs via the API. It's the DB update marking all purged HDAs as deleted that's a problem in my opinion.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2017

A bit of clarification, in the UI, there is no way to purge an undeleted HDA other than purging the history, or via the cleanup scripts.

I've checked the cleanup scripts and I cannot find a place where something non-deleted-yet is purged. Also the purge_histories() method deletes all history datasets.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

Interesting. pgcleanup.py does as I've described, but apparently the older cleanup script was never updated to do this. That's a bit concerning. The scripts should be updated to both do the same thing.

If nobody thinks it's useful to preserve the deleted state of HDAs in a deleted history then we can reinstate the change, but it seems kinda silly to make a conscious decision to permanently "lose" those details when the schema allows us not to.

@martenson

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

@natefoo just to be clear, the change is still in since the #3613 has not been merged yet

@jmchilton

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

@natefoo I'm not saying it isn't useful - it is. But there was clearly some cognitive costs or technical debt involved here since many people looked this over and no one saw a reason to keep it. I'm not -1 on the other PR - feel free to merge it - I just don't think it is a good idea.

@dannon

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

Just to chime in as well, since this is still floating -- even though I opened the other reversion PR (to have a discussion about it there) I do like the changes in this PR and don't think reverting it is a great idea. As I stated before, I'm skeptical there's value in keeping the additional complexity around.

@natefoo

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Since I seem to be the only person advocating for keeping deleted I say we keep this change and close #3613. However, pgcleanup.py also needs to be modified to set deleted accordingly.

@blankenberg

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

FWIIW, I think preserving the state is worthwhile.

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.