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

Add some debug info to diplomacy. #2577

Merged
merged 9 commits into from
Aug 14, 2020
Merged

Add some debug info to diplomacy. #2577

merged 9 commits into from
Aug 14, 2020

Conversation

sequencer
Copy link
Member

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes:
BundleBridgeNexusNode is using MixedNexusNode, debug without nearby nodes is hard.
This PR gives better debug infomation by printing nearby nodes require is false.

@sequencer sequencer requested a review from terpstra July 27, 2020 20:13
@sequencer sequencer requested a review from hcook August 9, 2020 04:57
@sequencer sequencer changed the title Give MixedNexusNode better debug info to locate unconnected nodes. Add some debug info to diplomacy. Aug 11, 2020
Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

Can you add similar information to

    require (po.size == oKnown || oStars == 1, s"$context has only ${oKnown} outputs connected out of ${po.size}")
    require (po.size >= oKnown, s"$context has ${oKnown} outputs out of ${po.size}; cannot assign ${po.size - oKnown} edges to resolve :=*")

in SourceNode
and

require (pi.size == iKnown || iStars == 1, s"$context has only ${iKnown} inputs connected out of ${pi.size}")
    require (pi.size >= iKnown, s"$context has ${iKnown} inputs out of ${pi.size}; cannot assign ${pi.size - iKnown} edges to resolve :*=")

in SinkNode?

I think those errors also cause confusion. Especially the phrasing "X inputs connected out of Y" because it sounds like Y should be bigger than X, but lots of times Y is smaller because you did not put enough params in your pi: Seq[U] 😅

Maybe "X inputs were connected ($names) but Y sinks were specified to the node constructor."?

Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

Maybe let's preface more (or all) of these types of errors with either "Diplomacy error:" or maybe something longer like val errorMessage = "Diplomacy detected a problem with your graph:\n"

These aren't bugs in diplomacy, and they aren't a protocol-specific requirethat failed, they are a problem with the structure of the graph that means negotiation will not be able to proceed... The answer is usually to go fix the shape of the graph to make it match the number of parameters put in or calculated...

@hcook
Copy link
Member

hcook commented Aug 12, 2020

@terpstra How do you feeling about preferring the terms "inward connections" and "outward connections" to "inputs" and "outputs", at least in terms of reporting these node error messages? I feel like "input" and "output" are kind of overloaded due to being used as wire IO directions...

@terpstra
Copy link
Contributor

Yes, inward and outward are better.

Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

One more general comment, is there some way we can get the SourceInfo information associated with a particular node's instantiation to show up in context or a particular binding to show up in oPorts/iPorts along with the name?

@sequencer
Copy link
Member Author

@hcook added more infos to resolveStar, I think this might be more easy for debugging?

@sequencer sequencer requested a review from hcook August 12, 2020 03:03
@sequencer sequencer requested a review from hcook August 13, 2020 07:19
Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

Thank you for all the edits! This is great!

@sequencer sequencer merged commit 6814964 into master Aug 14, 2020
@sequencer sequencer deleted the nexusnode_debug branch September 6, 2020 18:11
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.

3 participants