Reapply "[ty] Offer "Did you mean...?" suggestions for unresolved from imports and unresolved attributes (#18705)"
#18751
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CodSpeed Instrumentation Performance ReportMerging #18751 will degrade performances by 10.22%Comparing Summary
Benchmarks breakdown
|
Nice! (Sorry Alex) |
No, this is great!! It's fantastic that we're aware of this now! |
|
It might also just be that we issue this diagnostic now and do more work because of it? |
|
yeah this might be an "acceptable" regression if it only happens because there's a lot of unresolved imports and unresolved attributes in hydra-zen (and we now compute suggestions for every one of them). The easy way to speed ty up in that situation is to either fix the type errors or suppress them (either in configuration or suppression comments). But I'll nonetheless (1) see if I can repro the regression locally and (2) see if there are easy ways of speeding it up. |
I spot-checked the suggestions being given in the diagnostics here. Most of them seem self-evidently good! Some of them are a bit surprising to me, e.g. this one:
But all the surprising ones seem to match CPython's behaviour, so I think this is fine for an initial version of the feature. CPython's implementation of this feature has been very popular with users and doesn't get many bug reports these days. |
MichaReiser
reviewed
Jun 18, 2025
AlexWaygood
force-pushed
the
alex/reapply-levenshtein-2
branch
from
b845698 to
6d3b31c
Compare
I made that change in 2a0e9d4, but it doesn't seem to help with the performance regression locally (and the regression is easily reproducible locally) |
AlexWaygood
force-pushed
the
alex/reapply-levenshtein-2
branch
from
6d3b31c to
2a0e9d4
Compare
|
The slowdown on the hydra-zen benchmark is due to us attempting to calculate suggestions for We call Of the six And the entire slowdown on the benchmark is due to us attempting to calculate "Did you mean...?" suggestions for this diagnostic. This can be seen by the fact that applying this diff to my PR branch fixes the regression completely for me locally: diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index eed93853a5..cee63226c1 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -6574,12 +6574,14 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
HideUnderscoredSuggestions::Yes
};
- if let Some(suggestion) =
- find_best_suggestion_for_unresolved_member(db, value_type, &attr.id, underscore_policy)
- {
- diagnostic.set_primary_message(format_args!(
- "Did you mean `{suggestion}`?",
- ));
+ if value_type.into_module_literal().is_none_or(|module|module.module(self.db()).name() != "pydantic") {
+ if let Some(suggestion) =
+ find_best_suggestion_for_unresolved_member(db, value_type, &attr.id, underscore_policy)
+ {
+ diagnostic.set_primary_message(format_args!(
+ "Did you mean `{suggestion}`?",
+ ));
+ }
}
}
}The specific thing that appears to be expensive about calculating the "Did you mean...?" suggestion for this diagnostic is calculating the list of members the diff --git a/crates/ty_python_semantic/src/types/diagnostic/levenshtein.rs b/crates/ty_python_semantic/src/types/diagnostic/levenshtein.rs
index 4bd4a2c8c8..74031ff047 100644
--- a/crates/ty_python_semantic/src/types/diagnostic/levenshtein.rs
+++ b/crates/ty_python_semantic/src/types/diagnostic/levenshtein.rs
@@ -63,7 +63,7 @@ where
// Don't spend a *huge* amount of time computing suggestions if there are many candidates.
// This limit is fairly arbitrary and can be adjusted as needed.
- if options.len() > 4096 {
+ if options.len() > 128 {
return None;
}I can't see any obvious way of optimizing I still think the feature is worth the regression, since |
…an ancestor module or parent module of the current module
AlexWaygood
force-pushed
the
alex/reapply-levenshtein-2
branch
from
2a0e9d4 to
592f30f
Compare
AlexWaygood
changed the title
from imports and unresolved attributes (#18705)"
|
I do think (as I mentioned on the first iteration of the PR) that a likely consequence here is that our performance will look dramatically worse on real-world benchmarks on some codebases that haven't been adapted to pass diagnostic-free on ty. I agree with your analysis that in the long run perhaps this shouldn't concern us, but I think "looking slow" is a valid short-term concern. I think it might be sufficiently concerning that it would be worth adding a configuration option to disable this feature, so that at least we can tell people who say "ty is slow on my codebase", "hey, maybe try with this option and see if it helps." |
That makes sense to me — I'm open to that if others are! Would we want to just suppress "Did you mean...?" subdiagnostics specifically with this option (on the grounds that it's the only case where we have direct evidence that computing the subdiagnostic information can negatively impact performance) or would we want to suppress other (all?) subdiagnostics, on the grounds that lots of them do additional computation that could slow us down? Would you want this CLI option implemented as part of this PR or as a followup? |
|
Maybe that option already exists, and it's spelled I'm ok with that being done as follow-up, not in this PR. |
I think we currently do the same work whether |
|
The challenge with the output format is that a project can use consise but we still want to show all/more information in the lsp. So I don't think this will work. (Too late here to think about alternatives) |
|
The general problem with the suggestion here is that computing it can take an arbitrary long time, depending on how large the object is (it could be seconds!) I wonder if we should time box the computation or if there's some other way to limit the runaway risk. Either way; While very nice, I'm not sure if we should prioritize this right now |
Right — though since the expensive API ( |
That's somewhat different because it's okay for LSP actions to take longer. The client will send a cancellation request if the response is now outdated. So it's okay if LSP actions take longer (they're also in response to a user action). But obviously, they would ideally also not cause a run-away like this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR reapplies #18705, which was previously reverted in #18721.
#18705 was previously reverted because the original implementation of the feature caused catastrophic execution time on dd-trace-py, which meant that mypy_primer runs timed out on all PRs (they wouldn't finish within 20 minutes). The reason for the catastrophic execution time was cycles such as the following scenario:
foo/__init__.pyfrom . import barimport (infer_import_from_definition).tofoo/__init__.py, realizebaris an unknown symbol/submodule, and callall_members(<module 'foo'>)to calculate suggestions for the diagnosticall_memberstries to infer the types of all symbols in the global scope offoo/__init__.pydue to this call... and we're back at step (1)The cycles described above eventually resolved themselves due to our fixpoint iteration, but it led to a catastrophic execution time if it was a module with several
from . import <SUBMODULE>imports in it. Similar cycles could also manifest betweenfoo/__init__.pyfiles andfoo-package submodules:foo/whatever.pyfrom . import barall_members(module <'foo'>), which triggers us trying to infer global types forfoo/__init__.pyfoo/__init__.pyhas an importfrom .whatever import something_else, which is also unresolved. To compute suggestions for that unresolved import, we infer global types forfoo/whatever.py... and we're back at step (1)To avoid the catastrophic execution time when creating diagnostics for these cyclic imports, this PR refrains from attempting to compute suggestions for an
unresolved-importdiagnostic if the import is in the global scope and the import refers to module that is an ancestor or child of the module represented by the file currently being analyzed. This is implemented in 79f3c8c.Test Plan
fromimports and unresolved attributes #18705 have been added againThere is a performance regression on the hydra-zen benchmark. My analysis of this regression is in #18751 (comment). I think it's unavoidable if we want this feature, unfortunately.