-
Notifications
You must be signed in to change notification settings - Fork 208
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
Dumping: Check non-empty input node repositories #6734
base: main
Are you sure you want to change the base?
Conversation
2c24457
to
ebf6314
Compare
1709121
to
9e58538
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6734 +/- ##
==========================================
+ Coverage 78.09% 78.09% +0.01%
==========================================
Files 564 564
Lines 42544 42549 +5
==========================================
+ Hits 33219 33224 +5
Misses 9325 9325 ☔ View full report in Codecov by Sentry. |
all_input_nodes = input_links.all_nodes() | ||
all_have_repositories = all([hasattr(node.base, 'repository') for node in all_input_nodes]) | ||
if all_have_repositories: | ||
non_empty_repository = any([len(node.base.repository.list_objects()) > 0 for node in all_input_nodes]) | ||
if non_empty_repository: | ||
self._dump_calculation_io( | ||
parent_path=output_path / io_dump_mapping.inputs, | ||
link_triples=input_links, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all_input_nodes = input_links.all_nodes() | |
all_have_repositories = all([hasattr(node.base, 'repository') for node in all_input_nodes]) | |
if all_have_repositories: | |
non_empty_repository = any([len(node.base.repository.list_objects()) > 0 for node in all_input_nodes]) | |
if non_empty_repository: | |
self._dump_calculation_io( | |
parent_path=output_path / io_dump_mapping.inputs, | |
link_triples=input_links, | |
) | |
all_input_nodes = input_links.all_nodes() | |
if ( | |
all(hasattr(node.base, 'repository') for node in all_input_nodes) | |
and any(len(node.base.repository.list_objects()) for node in all_input_nodes) | |
): | |
self._dump_calculation_io( | |
parent_path=output_path / io_dump_mapping.inputs, | |
link_triples=input_links, | |
) |
nitpick: I think it can be simplified by merging two if (seems even hard to read, if you think so, feel free to ignore)?
Adding a small check to the process-dumping that incoming nodes actually do have associated
NodeRepository
s (that might actually not even be necessary, probably allNode
s have that by construction 🤔), and that these repositories actually do hold objects. This avoids emptynode_inputs
directories being created when dumping.