Improve the autocomplete feature of the builtin python IDE #8420
Conversation
|
@dimven can you try to improve the diff here? It looks like there are a bunch of white space only changes, or is that much of the code actually new? |
|
👍 Not sure why this happened and why git is detecting white spaces as a change. I must have "Ctrl+I"-ed the whole text at some point. I'll fix it over the weekend and resubmit |
|
Now that I'm back from the holidays, I did some more testing and still see the slowdowns and instability whenever I use a I don't observe the same when I use seems like the 'commaDelimitedVariableNamesRegex' is too greedy; need to change from: to: |
cleaned up the weird whitespace diffs.
|
Hi @dimven, thanks for tackling this. Any incremental improvements you can make to the autocompletion engine would be great! It also might be worth looking into replacing the entire internal engine with a complete Python autocompletion solution like Jedi -- it could in the end be less work than wrestling with regexes. Both Atom and VS Code use Jedi for their Python autocompletion. Here are the cores of their implementations: Atom's provider API is in Coffeescript, and VS Code's is in Typescript, while Jedi is implemented in Python itself. Both of these extensions just launch a Python process to run Jedi and communicate with it from the editor's language. I don't see any reason why the same couldn't be done from C#. Of course, this strategy is not without its pitfalls. It will probably be easiest to run Jedi in normal Python, and then you could run into challenges due to autocomplete running in normal Python while Dynamo actually runs IronPython. (In that case, this might help: https://github.com/gtalarico/ironpython-stubs) So perhaps this wouldn't necessarily be easier than just tweaking regexes, but I thought I'd mention it regardless! Another worthwhile line of investigation might be adding the ability to select an external editor for Python nodes, so editing them can pop up a window in your favorite IDE, which presumably already has Python autocompletion (though the normal Python vs. IronPython issues might still apply.) |
|
@Dewb @dimven @mjkkirschner I vote the very last suggestion 👍 |
|
I also vote for the last suggestions, had that discussion in another issue too : #6513 (comment) |
|
My view on this aligns with @andydandy74 's - unless Dynamo is properly set up and distributed with a default external IDE out of the box (i.e. how #develop environment used in Revit macros), it would be unpractical to completely forgo the current approach. Otherwise we're just asking for more deployment hell. Without a doubt, it would be great to have an easy to customize access to any IDE but that shouldn't compromise the experience of the majority of the users. On a side note, why was #6513 closed ? Last time I checked, nothing has been implemented for this in 2.0 |
Add and keep track of clr library references. That fixes a lot of the import fails later on
- Docstrings are automatically excluded from the code analysis. This is a really big improvement for cases where we handle large multiline string blocks in our code. - The autocomplete scope now picks up references to external assemblies and can support additional imports. - The default location of the python builtin library is added to the module search path and now those will be picked up too - Some of the regex syntax has been revised and instantiated as compiled to hopefully improve performance. TODO: - I'd like to further rework the regex syntax for variable detection and the import statements - Add support for basic variable unpacking(i.e. a, b, c = 1, 2, 3)
TODO:
|
- All completion tests are now outdated and need to be revised! - All regex statements have been revised for speed and responsiveness - import statement detection is much more robust and powerful and can have custom names - similarly, we now have variable assignment unpacking & other tweaks
|
@mjkkirschner just pushed a big update and would love to hear what you think about it:
The autocomplete now feels snappier and more responsive with larger scripts too:
|
variables referencing other variables are recognized correctly now
|
@dimven - This is awesome. We will review and get back to you. But just so you know, we are currently heads down on Dynamo 2.0, and this may have to wait for a 2.1 release. Thanks for your patience. We will get back to you soon. |
|
@Racel no hurry on my end :) . I'm pretty happy with how the new code behaves on my end and the feedback I got from others is positive, so I don't think I'll modify it any further for the time being. I also got around to implementing a very rudimentary overall autocomplete functionality:
but I'll first have to make sure it works well with the changes in 2.0 and will start a separate PR for it. |
|
@Racel do you have any forecast about when it will be released Dynamo 2.0? |
|
Would be great to have this in the next stable release... |
- avoids another exception thrown at incomplete import statements - adds additional functionality for enum types - minor fixes & refactoring
|
I will find time to look at this more closely as soon as I can. I'm sorry it has been open so long without a solid review. One thing I see immediately is the altering of public methods which we can't do without evaluating the risk - as it will break binary compatibility - we can keep these old public methods, mark them obsolete and create new ones though - and where possible we should be more conservative with access modifiers. Another initial thought is on the the loading of the standard library from the install directory - I think we were considering moving dynamo's python install into a dynamo sub folder to isolate it when updating to 2.7.8. Not sure that needs to be resolved in this PR though. |
johnpierson
added
priority
| /// <summary> | ||
| /// Maps a basic variable regex to a basic python type. | ||
| /// </summary> | ||
| public List<Tuple<Regex, Type>> BasicVariableTypes; |
There was a problem hiding this comment.
does this actually need to be public - can it be internal?
| /// <summary> | ||
| /// Tracks already referenced CLR modules | ||
| /// </summary> | ||
| public HashSet<string> clrModules { get; set; } |
| /// <summary> | ||
| /// Keeps track of failed statements to avoid poluting the log | ||
| /// </summary> | ||
| public Dictionary<string, int> badStatements { get; set; } |
| public static string variableName = @"([0-9a-zA-Z_]+(\.[a-zA-Z_0-9]+)*)"; | ||
| public static string doubleQuoteStringRegex = "(\"[^\"]*\")"; | ||
| public static string singleQuoteStringRegex = "(\'[^\']*\')"; | ||
| public static string commaDelimitedVariableNamesRegex = @"(([0-9a-zA-Z_]+,?\s?)+)"; |
There was a problem hiding this comment.
same question for some of these members - I know that the previous ones were all public, but unless this is actually useful to some external user - making these public - makes refactoring the code between major releases very painful.
| public static string singleQuoteStringRegex = "(\'[^\']*\')"; | ||
| public static string commaDelimitedVariableNamesRegex = @"(([0-9a-zA-Z_]+,?\s?)+)"; | ||
| public static string variableName = @"([0-9a-zA-Z_]+(\.[a-zA-Z_0-9]+)*)"; | ||
| public static string quotesStringRegex = "[\"']([^\"']*)[\"']"; |
There was a problem hiding this comment.
I think it would be very useful to add comments/summaries to some of these - especially if they are public to describe anything that is not obvious from the field names.
|
|
||
| #endregion | ||
|
|
||
| private static readonly Regex MATCH_LAST_NAMESPACE = new Regex(@"[\w.]+$", RegexOptions.Compiled); |
There was a problem hiding this comment.
I don't have a problem with the readonly fields being uppercase - but curious - is this from a specific style guide?
| } | ||
| catch | ||
| { | ||
| Log("Failed to load Revit types for autocomplete. Python autocomplete will not see Autodesk namespace types."); | ||
| Log("Failed to load Revit types for autocomplete. Python autocomplete will not see Autodesk namespace types."); |
There was a problem hiding this comment.
can we get more specific with this message?
| } | ||
| } | ||
|
|
||
| if (assemblies.Any(x => x.FullName.Contains("ProtoGeometry"))) | ||
| if (assemblies.Any(x => x.GetName().Name == "ProtoGeometry")) |
| } | ||
| } | ||
|
|
||
|
|
||
| string pythonLibDir = System.IO.Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86), |
There was a problem hiding this comment.
I'm just thinking this might change soon - but that should not hold up this pr.
| try | ||
| { | ||
| var pyLibImports = String.Format("import sys\nsys.path.append(r'{0}')\n", pythonLibDir); | ||
| engine.CreateScriptSourceFromString(pyLibImports, SourceCodeKind.Statements).Execute(scope); |
There was a problem hiding this comment.
I believe your previous work made the performance impact of this change minimal - is that true?
| /// <returns>Return a list of IronPythonCompletionData </returns> | ||
| public ICompletionData[] GetCompletionData(string line) | ||
| public ICompletionData[] GetCompletionData(string code, bool expand=false) |
There was a problem hiding this comment.
we can't change this methods signature at this time - even adding an optional parameter will break binary compatibility:
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net
It's very easy to miss cases here. :(
| } | ||
|
|
||
| /// <summary> | ||
| /// Generates completion data for the specified text, while import the given types into the | ||
| /// Generates completion data for the specified text, while import the given types into the |
There was a problem hiding this comment.
this comment does not seem grammatically correct.
| /// Find all import statements and import into scope. If the type is already in the scope, this will be skipped. | ||
| /// The ImportedTypes dictionary is | ||
| /// Find all import statements and import into scope. If the type is already in the scope, this will be skipped. | ||
| /// The ImportedTypes dictionary is |
| { | ||
| if (scope.ContainsVariable(import.Key)) | ||
| int previousTries = 0; |
| if (scope.ContainsVariable(import.Key)) | ||
| int previousTries = 0; | ||
| badStatements.TryGetValue(statement, out previousTries); | ||
| if (previousTries > 3) |
There was a problem hiding this comment.
what is the significance of the 3? Can it be pulled out to a constant?
| string libName = MATCH_FIRST_QUOTED_NAME.Match(statement).Groups[1].Value; | ||
| if (!clrModules.Contains(libName)) | ||
| { | ||
| if (statement.Contains("AddReferenceToFileAndPath")) |
There was a problem hiding this comment.
please pull this string out as a constant
| continue; | ||
| } | ||
|
|
||
| if(AppDomain.CurrentDomain.GetAssemblies().Any(x => x.GetName().Name == libName)) |
There was a problem hiding this comment.
hmm, maybe it is worth making a map of assemblies and checking if the name exists there instead of this loop - it appears lookup might happen a lot.
| } | ||
|
|
||
| var importStatements = FindAllImportStatements(code); | ||
| foreach (var i in importStatements) |
There was a problem hiding this comment.
can you add a comment about the general output/side effects of this for loop.
| /// </summary> | ||
| /// <param name="text"></param> | ||
| /// <returns></returns> | ||
| string GetName(string text) | ||
| string GetLastName(string text) |
There was a problem hiding this comment.
please add private access modifier
|
@dimven We think this looks pretty close and would like to get this into the next release. We would like to take this PR over, are you ok with that? |
|
Hi @Racel & @mjkkirschner Sorry, I've been real bussy the last few weeks and haven't had a chance to look at this again. I would absolutely love it if you could take over and wrap it up for 2.1 :) |
|
Picking this work back up in #9402 as per the discussion above. I am going to close this PR but feel free to continue any conversations/concerns in the new PR. |
Purpose
I've been having some instabilities when using the built in IDE with large scripts and for extended periods of time and I retraced that back to the current autocomplete implementation. I've revised the current implementation so that it avoids raising as many exceptions as it did. I then tried to improve the type guessing as much as possible.
The type guessing should be relatively accurate for simple statements. We can never guarantee a 100% coverage for complicated variable definitions, because we don't have the luxury of running in the same scope as the actual script or pre-executing the entire code of the script.
Here's a before capture; notice the constant exceptions being thrown in the console:
And this is with the proposed changes:
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner
FYIs
This entire class is pretty independent of the rest of Dynamo, so it should work fine in both current and future versions of Dynamo.