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

Provide resolution hints in case of possible local name conflicts #7505

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Sep 18, 2024

This enhances the hints generator in the resolver with some heuristic to detect and warn in case of failures due to version mismatches on a local package. Those may be the symptom of name conflict/shadowing with a transitive dependency.

Closes: #7329

@@ -539,6 +538,14 @@ impl PubGrubReportFormatter<'_> {
incomplete_packages,
output_hints,
);

// Check for no versions on the local packages themself, which may
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, this logic is too weak and gets tricked into false positives, I think from the root itself appearing as a normal package down the chain even on other kind of failures.
I think I need to enhance this heuristic in order to detect the cycle.

@lucab lucab force-pushed the ups/resolver-dependency-cycle branch from 4283b10 to 8c7bc4f Compare September 18, 2024 17:17
///
/// This shouldn't usually happen, but it could be the symptom of a naming
/// conflict with a transitive dependency existing on the index.
fn local_cycle_hint(
Copy link
Member

@zanieb zanieb Sep 19, 2024

Choose a reason for hiding this comment

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

Unfortunately this heuristic doesn't work for the invocation in the original report (I tested by dropping the pin from your test). Maybe this would be helped by transforming the tree in as tracked by #7524 but I'm not sure. It looks like it might just be a bug because the tree does still contain a cycle with the same structure.

Have you used the UV_INTERNAL__SHOW_DERIVATION_TREE=1 utility? It can be helpful to display the tree in it's raw form.

Without a version pin:

Resolver derivation tree after reduction
  dagster==0.0.0 depends on dagster-webserver*
    dagster-webserver==1.8.7 depends on dagster==1.8.7
       ... an enumeration of all the versions follows

With a version pin:

Resolver derivation tree before reduction
  root==0a0.dev0 depends on dagster*
      dagster==0.0.0 depends on dagster-webserver==1.6.13
      dagster-webserver==1.6.13 depends on dagster==1.6.13
    no versions of dagster<0.0.0 | >0.0.0
Resolver derivation tree after reduction
  dagster==0.0.0 depends on dagster-webserver==1.6.13
  dagster-webserver==1.6.13 depends on dagster==1.6.13

On a separate note, it's pretty hard to understand what this heuristic is doing. I know you plan to do some polish on it, and it's not your fault that the data structure is complicated, but I can't review it without some additional context on the idea.

This enhances the hints generator in the resolver with some heuristic
to detect and warn in case of failures due version mismatched on a
local package. Those may be the symptom of name conflict/shadowing
with a transitive dependency.
@lucab lucab force-pushed the ups/resolver-dependency-cycle branch from 8c7bc4f to 842f039 Compare September 19, 2024 17:51
@lucab lucab force-pushed the ups/resolver-dependency-cycle branch from 842f039 to 0defe6e Compare September 19, 2024 18:03
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.

Project's name interferes with resolution logic
2 participants