deprecate `--clear` option of `verdi group delete` #4357

ltalirz · 2020-09-04T15:23:21Z

ltalirz · 2020-09-04T15:26:20Z

I do have one more question about the purpose of clear()ing the group before deletion.

What is the difference between clearing the contents of a group and then deleting it vs directly deleting the group (using Group.objects.delete)?
As far as I understand, the nodes remain untouched in either case (I tried) - it seems like an unnecessary extra step.

The verdi user interface could remain the same in order to prevent users from accidentally deleting groups that contain nodes - although I would suspect that it is actually more common to use this command on groups that aren't empty.

sphuber · 2020-09-04T15:35:34Z

I added the clear option in 719d65a . It's functionality used to be combined with the --force flag, which I though was potentially dangerous. It is true though that it should just serve as a barrier to people accidentally deleting non empty groups, there is no need to call group.clear. Could you update the PR to include this change? I think that would be fine.

codecov · 2020-09-04T15:39:50Z

Codecov Report

Merging #4357 (2830206) into develop (c42a86b) will increase coverage by 0.02%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           develop    #4357      +/-   ##
===========================================
+ Coverage    79.51%   79.52%   +0.02%     
===========================================
  Files          482      482              
  Lines        35327    35325       -2     
===========================================
+ Hits         28085    28088       +3     
+ Misses        7242     7237       -5

Flag	Coverage Δ
django	`73.67% <80.00%> (-<0.01%)`	⬇️
sqlalchemy	`72.86% <80.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_group.py	`87.23% <80.00%> (-0.54%)`	⬇️
aiida/transports/plugins/local.py	`81.54% <0.00%> (+0.26%)`	⬆️
aiida/orm/utils/log.py	`93.34% <0.00%> (+16.67%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c42a86b...2830206. Read the comment docs.

ltalirz · 2020-09-04T15:40:31Z

Thanks, I will make the change.

Just wondering, should there also be a flag to actually delete the nodes as well?
It seems to me that this would be quite useful in practice (although it should obviously be off by default).

sphuber · 2020-09-04T15:44:53Z

Just wondering, should there also be a flag to actually delete the nodes as well?
It seems to me that this would be quite useful in practice (although it should obviously be off by default).

Think that makes sense, however, it should come with a confirmation prompt, because it will have to follow the consistency rules and it may need to delete a lot more nodes than just those that were in the group.

ltalirz · 2020-09-07T12:25:51Z

Just wondering, should there also be a flag to actually delete the nodes as well?
It seems to me that this would be quite useful in practice (although it should obviously be off by default).

Think that makes sense, however, it should come with a confirmation prompt, because it will have to follow the consistency rules and it may need to delete a lot more nodes than just those that were in the group.

Great - I'll open an issue for this, since it is a bit beyond the scope of this PR.
The PR should be ready to go.

sphuber · 2020-09-09T21:09:39Z

@mbercx will you have time to do the implementation of #4358 relatively soon? Because that would make these changes obsolete anyway.

mbercx · 2020-09-10T06:58:07Z

@mbercx will you have time to do the implementation of #4358 relatively soon? Because that would make these changes obsolete anyway.

Sure, I should be able to work on this early next week! So I guess we can close this PR?

ltalirz · 2020-09-10T07:32:02Z

I'd would still merge it. You anyhow won't be able to remove the --clear flag before 2.0, i.e. the docs will still need to mention it.

sphuber · 2020-09-10T07:39:46Z

Fine by me to postpone the new interface until 2.0 since we are changing it, but then it would be good to put official deprecation warning for the clear option in this PR. If you can add that then it's fine for me to merge

ltalirz · 2020-09-15T21:48:44Z

then it would be good to put official deprecation warning for the clear option in this PR.

I agree. Just to clarify, though: this means I will also remove the necessity to specify the --clear option (we don't want to be forcing people to use a deprecated option).
I think this change in functionality is ok - it should not break existing scripts.

sphuber · 2020-09-16T07:56:34Z

ust to clarify, though: this means I will also remove the necessity to specify the --clear option (we don't want to be forcing people to use a deprecated option).
I think this change in functionality is ok - it should not break existing scripts.

I agree that it is not optimal, but I am not sure I agree with the assessment. Some people may actually have come to rely on the behavior, calling it without --clear because they only want to delete empty groups. If we silently remove that safety, that breaks that assumption. "Scripts will continue to work" but maybe not as intended. So I would actually keep the option and update the deprecation warning to say "the --clear option is deprecated and will be removed in 2.0 after which empty and non-empty groups will be treated equally"

mbercx · 2020-10-29T15:49:28Z

@ltalirz should we still add this deprecation in the 1.5.0 release (#4516)? It's important to add this asap, so we can include #4425 in 2.0.

I do tend to agree with @sphuber that we should not just remove the options functionality, but just add a deprecation warning.

ltalirz · 2020-10-29T22:59:33Z

Right - I certainly get the argument (this change could in principle break user's scripts).
The reason I am a bit hesitant to implement this suggestion is that this is a somewhat special case: the suggestion here is to mark an API as "to be removed" without giving users an alternative. I.e. by deprecating it we force any user who wants to delete a non-empty group to read our deprecation warning until 2.0 is out, and we're forcing them to keep using an API that will break in 2.0 (meaning their scripts will be guaranteed to break).

The reason we are doing this is to cover the potential case of users relying on the fact that leaving out the --clear option will prevent them from deleting non-empty groups (I would add that this concern only applies to --non-interactive use, since without the --non-interactive flag there is still a confirmation prompt).
Granted - it is possible that users use verdi group delete -n and rely on the fact that this will fail when a group is not empty but I would bet that I can count the number of users who do this on one hand (if it isn't zero), while the number of users impacted by the situation described in the first paragraph is much larger.

So, in my view it's a balance to strike here and I lean towards changing the behavior so that people have time to adapt their code to remove the --clear flag in preparation of the 2.0 release.

sphuber · 2020-10-30T07:52:49Z

So, in my view it's a balance to strike here and I lean towards changing the behavior so that people have time to adapt their code to remove the --clear flag in preparation of the 2.0 release.

That's exactly the point though. How will you get people to stop using it, without having it print a deprecation warning?

By the way, there is no --non-interacitve option, I presume you meant --force. I think a deprecation warning on the command line is not that big of a deal and at this point we can no longer really argue like "I don't think there is anyone using this". I am also tempted to use this reasoning to be looser with deprecation pathways, but I think we start to have a large group of users now that we don't speak to directly and so have no way of knowing of how they use AiiDA.

ltalirz · 2020-10-30T09:07:01Z

That's exactly the point though. How will you get people to stop using it, without having it print a deprecation warning?

Sorry if I'm not being clear - I am proposing the current state of the PR, where we of course have a deprecation warning (but we are also changing the behavior of the command without the --clear option).
It's deprecating --clear with a way out (=stop using it).

By the way, there is no --non-interacitve option, I presume you meant --force

Right, sorry.

we can no longer really argue like "I don't think there is anyone using this". I am also tempted to use this reasoning to be looser with deprecation pathways, but I think we start to have a large group of users now that we don't speak to directly and so have no way of knowing of how they use AiiDA.

Well, my point was that we are not offering a way out until 2.0 and that there are definitely going to be a significant number of people affected by this (basically anyone who uses the verdi group delete command), who will then be forced to adapt immediately with 2.0 rather than gradually starting today.

ltalirz · 2020-11-09T20:41:43Z

@mbercx I believe this is still waiting for your review

mbercx

Sorry for the delay, @ltalirz! I have just one small suggestion.

mbercx · 2020-11-11T08:14:30Z

aiida/cmdline/commands/cmd_group.py

-            )
-        ))
+    if clear:
+        warnings.warn('`--clear` is deprecated and no longer has any effect.', AiidaDeprecationWarning)  # pylint: disable=no-member



Perhaps we can still add a warning in case the group is not empty and the force flag isn't set?

Suggested change

if group.count() > 0 and not force:

echo.echo_warning(f'Group <{label}> contains {group.count()} nodes.')

N.B.: .count() can potentially be expensive, so it's better to call it once and assign to variable

thanks @mbercx - I added it but then I had second thoughts

Warning people about how many nodes there are inside the group can give the impression that we're deleting nodes here (while it is just the group that is being deleted)

As @sphuber mentions, it can slow down the command

I.e. unless you feel strongly about this, I prefer to leave the code as it is

Warning people about how many nodes there are inside the group can give the impression that we're deleting nodes here (while it is just the group that is being deleted)

That's a good point, I hadn't thought about that. I suppose we can still rephrase the warning, but it's not critical.

As @sphuber mentions, it can slow down the command

I actually tested this for one of our larger groups on theossrv5, and it's not that time consuming:

In [10]: g.count() Out[10]: 61134 In [11]: timeit g.count() 174 ms ± 1.39 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

But, I do not feel super strongly about it, so feel free to merge!

this may be on the sqlalchemy backend, where I remember trying to optimize the shit out of it because it was really slow at some point and it shouldn't be. could be different on Django, could be also ok though, not sure. But I am also fine with leaving it as is as @ltalirz suggests. Let's get this thing merged.

ltalirz · 2020-11-11T16:45:26Z

Thanks @mbercx !

docs: clarify verdi node delete docstring

9b4eb17

remove unnecessary Group.clear()

45ed3b2

sphuber changed the title ~~docs: clarify verdi node delete docstring~~ docs: clarify verdi group delete docstring Sep 4, 2020

ltalirz mentioned this pull request Sep 7, 2020

Add option to verdi group delete to delete nodes themselves #4358

Closed

ltalirz requested a review from sphuber September 7, 2020 12:41

ltalirz changed the title ~~docs: clarify verdi group delete docstring~~ deprecate --clear option of verdi group delete Sep 15, 2020

deprecate --clear flag and remove any functionality

1d061a6

ltalirz force-pushed the issue_4356_group_delete_docstring branch from 4b9b91f to 1d061a6 Compare September 15, 2020 21:56

mbercx requested changes Nov 11, 2020

View reviewed changes

mbercx self-requested a review November 11, 2020 16:43

mbercx approved these changes Nov 11, 2020

View reviewed changes

Merge branch 'develop' into issue_4356_group_delete_docstring

2830206

sphuber merged commit 008580e into aiidateam:develop Nov 11, 2020

sphuber added this to the v1.5.0 milestone Nov 11, 2020

sphuber deleted the issue_4356_group_delete_docstring branch November 11, 2020 17:05

ltalirz mentioned this pull request Jan 27, 2021

Docs: docstring of verdi group delete command can be confusing #4356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate `--clear` option of `verdi group delete` #4357

deprecate `--clear` option of `verdi group delete` #4357

ltalirz commented Sep 4, 2020 •

edited

Loading

ltalirz commented Sep 4, 2020 •

edited

Loading

sphuber commented Sep 4, 2020

codecov bot commented Sep 4, 2020 •

edited

Loading

ltalirz commented Sep 4, 2020

sphuber commented Sep 4, 2020

ltalirz commented Sep 7, 2020

sphuber commented Sep 9, 2020

mbercx commented Sep 10, 2020

ltalirz commented Sep 10, 2020

sphuber commented Sep 10, 2020

ltalirz commented Sep 15, 2020

sphuber commented Sep 16, 2020

mbercx commented Oct 29, 2020 •

edited

Loading

ltalirz commented Oct 29, 2020 •

edited

Loading

sphuber commented Oct 30, 2020

ltalirz commented Oct 30, 2020 •

edited

Loading

ltalirz commented Nov 9, 2020

mbercx left a comment

mbercx Nov 11, 2020

sphuber Nov 11, 2020

ltalirz Nov 11, 2020 •

edited

Loading

mbercx Nov 11, 2020

sphuber Nov 11, 2020

ltalirz commented Nov 11, 2020


	if group.count() > 0 and not force:
	echo.echo_warning(f'Group <{label}> contains {group.count()} nodes.')

deprecate --clear option of verdi group delete #4357

deprecate --clear option of verdi group delete #4357

Conversation

ltalirz commented Sep 4, 2020 • edited Loading

ltalirz commented Sep 4, 2020 • edited Loading

sphuber commented Sep 4, 2020

codecov bot commented Sep 4, 2020 • edited Loading

Codecov Report

ltalirz commented Sep 4, 2020

sphuber commented Sep 4, 2020

ltalirz commented Sep 7, 2020

sphuber commented Sep 9, 2020

mbercx commented Sep 10, 2020

ltalirz commented Sep 10, 2020

sphuber commented Sep 10, 2020

ltalirz commented Sep 15, 2020

sphuber commented Sep 16, 2020

mbercx commented Oct 29, 2020 • edited Loading

ltalirz commented Oct 29, 2020 • edited Loading

sphuber commented Oct 30, 2020

ltalirz commented Oct 30, 2020 • edited Loading

ltalirz commented Nov 9, 2020

mbercx left a comment

Choose a reason for hiding this comment

mbercx Nov 11, 2020

Choose a reason for hiding this comment

sphuber Nov 11, 2020

Choose a reason for hiding this comment

ltalirz Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

mbercx Nov 11, 2020

Choose a reason for hiding this comment

sphuber Nov 11, 2020

Choose a reason for hiding this comment

ltalirz commented Nov 11, 2020

deprecate `--clear` option of `verdi group delete` #4357

deprecate `--clear` option of `verdi group delete` #4357

ltalirz commented Sep 4, 2020 •

edited

Loading

ltalirz commented Sep 4, 2020 •

edited

Loading

codecov bot commented Sep 4, 2020 •

edited

Loading

mbercx commented Oct 29, 2020 •

edited

Loading

ltalirz commented Oct 29, 2020 •

edited

Loading

ltalirz commented Oct 30, 2020 •

edited

Loading

ltalirz Nov 11, 2020 •

edited

Loading