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

Process: raise when exposed_outputs gets non-existing namespace #5265

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 9, 2021

Fixes #5261

Up till now, a call to exposed_outputs providing a namespace as
argument that doesn't exist, i.e., no outputs were ever exposed in that
namespace, would simply be a no-op. Given that this would often be
because the namespace was accidentally misspelled, the error would go
unnoticed and the process would finish without error message.

To prevent this situation that is difficult to debug, the method will
now raise a KeyError if the top-level namespace does not exist.

@sphuber sphuber force-pushed the fix/5261/exposed-outputs-non-existing-namespace branch from def7426 to a41716c Compare December 10, 2021 10:01
@codecov
Copy link

codecov bot commented Dec 10, 2021

Codecov Report

Merging #5265 (24fc5c0) into develop (d9f8c85) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5265      +/-   ##
===========================================
+ Coverage    82.02%   82.02%   +0.01%     
===========================================
  Files          533      533              
  Lines        38255    38257       +2     
===========================================
+ Hits         31373    31376       +3     
+ Misses        6882     6881       -1     
Flag Coverage Δ
django 77.07% <100.00%> (+0.01%) ⬆️
sqlalchemy 76.36% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
aiida/engine/processes/process.py 91.77% <100.00%> (+0.04%) ⬆️
aiida/transports/plugins/local.py 81.71% <0.00%> (+0.26%) ⬆️

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 d9f8c85...24fc5c0. Read the comment docs.

Up till now, a call to `exposed_outputs` providing a `namespace` as
argument that doesn't exist, i.e., no outputs were ever exposed in that
namespace, would simply be a no-op. Given that this would often be
because the namespace was accidentally misspelled, the error would go
unnoticed and the process would finish without error message.

To prevent this situation that is difficult to debug, the method will
now raise a `KeyError` if the top-level namespace does not exist.
@sphuber sphuber force-pushed the fix/5261/exposed-outputs-non-existing-namespace branch from a41716c to 5329524 Compare December 10, 2021 10:24
@sphuber
Copy link
Contributor Author

sphuber commented Dec 10, 2021

@janssenhenning I think this should fix the problem as now it will throw an exception. Let us know what you think of that solution.

@janssenhenning
Copy link
Contributor

@sphuber Thank you for the quick fix. Judging from the test this is the behaviour I would like 👍.
I have one question: Does this also prevent the case, where the namespace argument is missing from the exposed_outputs call but the output was exposed with a namespace? If not, would it also be possible to catch this case?

@sphuber
Copy link
Contributor Author

sphuber commented Dec 14, 2021

Does this also prevent the case, where the namespace argument is missing from the exposed_outputs call but the output was exposed with a namespace? If not, would it also be possible to catch this case?

No it does not. This would also be more tricky to do. For example, imagine a process that exposes a process without namespace and a process with a namespace. If the parent process then calls exposed_outputs without namespace on accident, there is no way to know for the code whether this was actually an accident or intended. I would say that this case is therefore not really possible to solve in a robust and general way.

@janssenhenning
Copy link
Contributor

Okay I see. The current solution seems fine to me

@sphuber
Copy link
Contributor Author

sphuber commented Jan 13, 2022

@ramirezfranciscof could you give this a look? It is not a big one and would be good to get merged for v2.0

@sphuber
Copy link
Contributor Author

sphuber commented Jan 17, 2022

@chrisjsewell @ramirezfranciscof let me know if you want to still give this a look. Otherwise I am merging tomorrow

@sphuber sphuber merged commit fb5a4d4 into aiidateam:develop Jan 18, 2022
@sphuber sphuber deleted the fix/5261/exposed-outputs-non-existing-namespace branch January 18, 2022 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing/misspelled namespace argument to exposed_outputs leads silently dismissing outputs
2 participants