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-34811 Fix component linkages in pipeline dot render #185

Merged
merged 4 commits into from May 25, 2022

Conversation

natelust
Copy link
Contributor

Creating a dot graph was previously not correctly linking component
datasets to the composite that provided them when drawing a graph,
this ensures the linkages are added appropriately.

Checklist

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

Creating a dot graph was previously not correctly linking component
datasets to the composite that provided them when drawing a graph,
this ensures the linkages are added appropriately.
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #185 (475179c) into main (963e35b) will decrease coverage by 0.09%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
- Coverage   82.67%   82.57%   -0.10%     
==========================================
  Files          47       47              
  Lines        3653     3673      +20     
  Branches      591      596       +5     
==========================================
+ Hits         3020     3033      +13     
- Misses        465      470       +5     
- Partials      168      170       +2     
Impacted Files Coverage Δ
python/lsst/ctrl/mpexec/dotTools.py 48.87% <80.76%> (+3.65%) ⬆️
tests/test_dotTools.py 96.05% <100.00%> (+0.10%) ⬆️
tests/test_executors.py 91.07% <0.00%> (-0.60%) ⬇️

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 963e35b...475179c. Read the comment docs.

Copy link
Collaborator

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks good, one minor comment.

# connect component dataset types to the composite type that
# produced it
if component is not None and (nodeName, attr.name) not in allDatasets:
_renderEdge(nodeName, attr.name, file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also need:

if nodeName not in allDatasets:
    _renderDSTypeNode(nodeName, ...)

(in case a component is a pre-existing input and not an intermediate)?

It may be a nicer representation if a component could be rendered as a box inside a dataset type node (e.g. using record-based graphviz nodes), but that is probably out of scope for this ticket.

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 am not sure what you mean with your first point, the rendering of the DSType node is handled above, and prerequisites are handled in a separate loop.

I thought about your latter point, but decided it was not worth re-working so much for a quicker fix that seems to do its job well enough.

Copy link
Collaborator

@andy-slac andy-slac May 18, 2022

Choose a reason for hiding this comment

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

I think the code above renders a node for attr.name, but here you add an edge from nodeName to attr.name. If there is no node rendered for nodeName it will be shown using default node representation without any dataset-related info (simple box with a name inside).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, I see what you are saying, good point, ill fix that up

Create connections to corresponding task which produces a metadata
dataset if it is specified as an input connection
Sort the contents that are used to generate the dot file such that
the output comes out deterministic.
Copy link
Collaborator

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks good, couple of minor comments.

python/lsst/ctrl/mpexec/dotTools.py Outdated Show resolved Hide resolved
python/lsst/ctrl/mpexec/dotTools.py Outdated Show resolved Hide resolved
_renderDSTypeNode(nodeName, dimensions, file)
# The next if block is a workaround until DM-29658 at which time
# metadata connections should star working with the above code
if (match := re.match("^(.*)_metadata$", attr.name)) is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe compile the regex to save few CPU cycles?

python/lsst/ctrl/mpexec/dotTools.py Outdated Show resolved Hide resolved
Fix typos in doc-strings and optimize regular expression use.
@natelust natelust merged commit c3cc6a3 into main May 25, 2022
@natelust natelust deleted the tickets/DM-34811 branch May 25, 2022 19:34
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

2 participants