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

DM-36145: Add additional quanta information for pipetask run #285

Merged
merged 3 commits into from Oct 12, 2022

Conversation

leeskelvin
Copy link
Contributor

@leeskelvin leeskelvin commented Oct 10, 2022

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Base: 81.80% // Head: 81.79% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (f0489bb) compared to base (33d0155).
Patch coverage: 66.66% of modified lines in pull request are covered.

❗ Current head f0489bb differs from pull request most recent head b770da9. Consider uploading reports for the commit b770da9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   81.80%   81.79%   -0.02%     
==========================================
  Files          56       56              
  Lines        6145     6147       +2     
  Branches     1126     1126              
==========================================
+ Hits         5027     5028       +1     
- Misses        886      887       +1     
  Partials      232      232              
Impacted Files Coverage Δ
python/lsst/pipe/base/graph/graph.py 81.30% <66.66%> (-0.18%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -440,7 +440,7 @@ def getQuantaForTask(self, taskDef: TaskDef) -> FrozenSet[Quantum]:
The `set` of `Quantum` that is associated with the specified
`TaskDef`.
"""
return frozenset(node.quantum for node in self._taskToQuantumNode[taskDef])
return frozenset(node.quantum for node in (self._taskToQuantumNode.get(taskDef) or tuple()))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's dict-like, self._taskToQuantumNode.get(taskDef, ()) or similar should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried your suggestion, and it does work as intended. With that said, I feel like the syntax here is more explicit about what it's doing, which I prefer. My preference would be to keep it as-is (and similarly below in the new method that's been added), but if you feel strongly otherwise, I'd be happy to change it.

Copy link

@taranu taranu Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't matter in this case because I wouldn't expect the get call to return anything False-but-not-None, but they're not identical. The current form would ignore any Falsey values (literal False, 0, etc.), which I would generally find to be a strange choice if deliberate. Consider x = {'a': 0} followed by if y := x.get('a'): print(y); this prints nothing.

Copy link
Member

@timj timj Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would tend to agree that this code is saying that the result should be a tuple if there is no element in the dict and so .get(x, DEFAULT) is closer to that intent. It's not really saying that tuple should be returned if any of the elements evaluate to false.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's .get() either way. I'm saying I can't think of a good reason to use dict.get(key) or default instead of dict.get(key, default), unless you really want to change the return type for a particular False-y value like an empty dict to a list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was agreeing with you 100% but my phrasing was ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both. Both relevant lines edited as per your recommendation.

Add an interface to return the number of quanta associated with
a given taskDef.
@leeskelvin leeskelvin merged commit b7c38f7 into main Oct 12, 2022
@leeskelvin leeskelvin deleted the tickets/DM-36145 branch October 12, 2022 00:46
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.

None yet

4 participants