Fix verdi calcjob cleanworkdir command
#5209
Conversation
|
Also, since cleaning hundres of calculations can be very slow, is it worth storing a flag for those that has been cleaned in |
Codecov Report
@@ Coverage Diff @@
## develop #5209 +/- ##
===========================================
+ Coverage 82.03% 82.06% +0.04%
===========================================
Files 533 533
Lines 38281 38296 +15
===========================================
+ Hits 31399 31423 +24
+ Misses 6882 6873 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks a lot @zhubonan
Sorry for the delay in reviewing it.
I have some minor questions about the implementation. The exit_status part looks good to me, but not pretty sure the past and older options should be applied at the same time for filtering.
|
Hi @zhubonan, do you get a chance to have a look and finish this PR. If you need any help please let me know, I also require this feature to clear the remote folder. |
|
Hi @unkcpz sorry for leaving it for a while! Just replied to your comments. I think it is almost ready to merge now |
unkcpz
force-pushed
the
fix-cleanworkdir
branch
from
77ef144
to
0d371a3
Compare
There was a problem hiding this comment.
Cheers! All tests passed after the outstanding fix on CI. I add a minor commit to pass the verdi performance test fail I hope you not mind @zhubonan.
I am all good with the changes except the --include-cleaned option. pinning @ramirezfranciscof for the votes.
A `Transport` is created for a `Computer`, but to keep the transport class independent of the `Computer` ORM class, it is not passed as an argument. This means however that, given a transport instance, it was not possible to know for which `Computer` it was constructed, which is sometimes necessary for consistency checks. When the transport is constructed the hostname of the `Computer` is passed as the `machine` keyword argument, and normally the hostname should identify the computer uniquely. Note that there is still the question of for which user the transport is configured, but that is currently not taken into account.
sphuber
force-pushed
the
fix-cleanworkdir
branch
from
b2049b5
to
d5a5920
Compare
|
@zhubonan I realized that due to my requests, the scope of the PR had broadened considerably and so I thought it best to have separate commits for clarity. Since I didn't want to burden you with this additional work, I took the liberty to rebase it myself. In the process I also updated the tests for |
|
@sphuber No problem at all. Please free feel to edit, rebase and merge. |
Allowing to pass an already open `Transport` to the `_clean` method, prevents having to open the same transport multiple times when cleaning many `RemoteData` nodes on the same computer. The downside is that it is possible for a user to pass a transport that is configured for a computer that is not the one of the `RemoteData`. A check is added to prevent this, but it only checks the hostname as it cannot check the user for which the transport is configured. In addition to this efficiency update, if the clean operation is successful, an extra with the key `KEY_EXTRA_CLEANED` is set to `True`. This is useful to identify `RemoteData` nodes that have been cleaned. Co-Authored-By: Sebastiaan Huber <mail@sphuber.net>
The command did not apply the `AND` clause when both the `-o` and the `-p` options were provided simultaneously and only the former would be applied. This is now corrected. On top of that, a new option `-E/--exit-status` is added that allows to filter on a specific exit status of the calculation jobs.
sphuber
force-pushed
the
fix-cleanworkdir
branch
from
d5a5920
to
1a1363c
Compare
|
Thanks a lot @zhubonan ! |
Previously the AND condition is not apply when -o and -p are both supplied, only the former is applied....
Also adds the ability to filter by
exit_statusof the calcjob.