-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[core] Refactoring variable management and expression evaluation for performances #2888
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors variable management and expression evaluation to improve performance by separating expression variables from command line variables and optimizing internal folder path computations.
- Separated expression evaluation (
_expVars) from command line generation (createCmdLineVars()) - Optimized internal folder path computation by removing redundant cache directory joins
- Added static expression variables for commonly used values like
nodeTypeandnodeSourceCodeFolder
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| meshroom/core/node.py | Core refactoring separating expression variables from command line generation, optimizing path computations |
| meshroom/core/desc/node.py | Updated command line building to use new variable separation approach |
| meshroom/core/attribute.py | Updated to use new expression variables and added error handling for malformed expressions |
| tests/test_nodeCommandLineFormatting.py | Updated test assertions to use new _expVars instead of _cmdVars |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (64.28%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #2888 +/- ##
===========================================
- Coverage 79.41% 79.06% -0.36%
===========================================
Files 51 51
Lines 6987 7007 +20
===========================================
- Hits 5549 5540 -9
- Misses 1438 1467 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7c7d918 to
98889c6
Compare
Adding cacheDir before an absolution path does nothing. os.path.join(absPathA, absPathB, filename) gives absPathB/filename.
- Command line vars are now only computed during the command line
generation for the "CommandLineNode".
- internalFolder is now initialized in the BaseNode, instead of Node
(even if there is the particular case of "InputNode" which does not need
it). And its value is computed once in the updateInternals instead of
redoing the string format again and again.
- internalFolder is not serialized/deserialized anymore as it is a
static information. At the beginning, it was designed to be customized
per node, but this flexibility/complexity seems useless as it has never
been used over many years. We now use "{nodeCacheFolder}" directly.
Make a clear difference between the variables needed for expressions and the ones needed for the command line nodes.
…ssion during deserialization
98889c6 to
c95c257
Compare
[meshroom] `CameraInit`: Remove `_cmdVars` As a consequence of alicevision/Meshroom#2888, _cmdVars, which was used in CameraInit, does not exist anymore and node.internalFolder now contains the resolved path. There is thus no need to format it anymore.
[core] Correctly set `_sourceCodeFolder` in `BaseNode`'s constructor This PR fixes a regression introduced by the refactoring of the command line variables in #2888, where the static nodeSourceCodeFolder is initialized but never set with the correct value once it becomes available. This is fixed by initializing _sourceCodeFolder directly in the BaseNode's constructor if the description is available (if it isn't, the node will be in Unknown Node compatibility error anyway). This both simplifies what was done prior to #2888 (where _sourceCodeFolder was correctly set later than it should have been) and ensures that _staticExpVars["nodeSourceCodeFolder"] is initialized with the correct value as well. Additionally, simple unit tests are added to ensure that static variables are always correctly set. A test InputNode is also added to one of the test plugins to cover all cases.
cmdVarshas been splitted between:expVars,staticExpVarsandcmdLineVars.generation for the "CommandLineNode".
(even if there is the particular case of "InputNode" which does not need
it). And its value is computed once in the updateInternals instead of
redoing the string format again and again.
static information. At the beginning, it was designed to be customized
per node, but this flexibility/complexity seems useless as it has never
been used over many years. We now use "{nodeCacheFolder}" directly.