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

should generate_graph_children() filter out abs deps from other cycles #6290

Closed
oliver-sanders opened this issue Aug 7, 2024 · 4 comments
Closed

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 7, 2024

There's an if statement in the code that filters out absolute dependencies from other cycles.

It's not entirely clear why this statement was added, the corresponding statement in generate_graph_parents was removed, should this one be removed too?

See this comment:

What about this equivalent bit in generate_graph_children()?

if is_abs and trigger.get_parent_point(point) != point:
# If 'foo[^] => bar' only spawn off of '^'.
continue

Originally posted by @MetRonnie in #6103 (comment)

  • Are there any consequences to this statement? Are there graphs where this could cause a bug?
  • Are there any consequences to removing this statement? E.G. Does it impact performance, causing double counting?

@dwsutherland, any insights welcome.

@dwsutherland
Copy link
Member

Not entirely sure.. Because the children of [^] are potentially infinite, where the parents are not?
Don't think I was the original author (although possible?)

@oliver-sanders
Copy link
Member Author

Not entirely sure.. Because the children of [^] are potentially infinite, where the parents are not?

That makes good sense!

@oliver-sanders
Copy link
Member Author

With the change to grah-parents, abs deps seem to be working as expected, @MetRonnie, please close if you're happy with this explanation.

@MetRonnie
Copy link
Member

Ok I've managed to figure out a case where this continue is hit

        P1 = foo
        R1/2 = foo[1] => pub

Without this continue, it results in edges that shouldn't exist (only 1/foo -> 2/pub should exist):

image

@MetRonnie MetRonnie closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2024
@oliver-sanders oliver-sanders removed this from the some-day milestone Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants