| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 666
DYN-9537: Enhance DynamoUnits with auto schema discovery #16529
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9537
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 enhances the DynamoUnits schema loading system to implement automatic discovery of Autodesk Shared Components (ASC) schema installations as alternative schema sources. Previously, Dynamo only used bundled v1.0.4 schemas. The change implements a candidate directory approach that prioritizes ASC schemas from newer product releases, falling back to bundled schemas when ASC schemas are unavailable.
Key changes:
- Implements candidate directory discovery system with priority ordering
- Integrates ASC schema detection using
AscSdkWrapperwith proper version sorting - Adds schema directory validation to ensure completeness before loading
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
AssemblyInfo.cs |
Adds InternalsVisibleTo attribute for DynamoUnits assembly access |
AscSDKWrapper.cs |
Enhances GetMajorVersions() to return versions sorted newest-first |
Utilities.cs |
Major refactor implementing candidate directory discovery, ASC integration, and schema validation |
UnitsCore.csproj |
Adds project reference to DynamoInstallDetective for ASC functionality |
|
@benglin what do you think about the option to also not ship a fallback? Geometry does not operate without ASC. Similar could be applied to Units. Not sure if that is necessary. Also could introduce an option to utilize the nuget package that we include in the current build. |
|
@benglin I believe that the current implementation supports using the latest schema version. For example, file save with unit-1.0.0 but now the backing schema is unit-2.0.0. Can you confirm |
|
Hi @saintentropy, thank you for taking the time to review these changes -- I appreciate your feedback. Here are my thoughts on your points: On the fallback schemas: Dynamo provides unit conversion nodes that users depend on, including in sandbox mode. Removing the fallback (default) schemas would likely break these workflows. When you refer to the "nuget package that we include in the current build", that's actually the default schema we use as a fallback. I assume you’re not suggesting we remove this fallback mechanism, but please let me know if I’ve misunderstood. You also make a great point about adaptive schema versioning. I had taken screenshots to illustrate this but neglected to include them in the PR description. I’ve now added those screenshots, along with a detailed explanation of how the adaptive and fallback unit system works in Dynamo. Please take a look -- these updates should address your questions. |
| <ProjectReference Include="..\..\NodeServices\DynamoServices.csproj"> | ||
| <Private>False</Private> | ||
| </ProjectReference> | ||
| <ProjectReference Include="..\..\Tools\DynamoInstallDetective\DynamoInstallDetective.csproj" /> |
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.
I think this is going to cause issues trying to run these nodes on linux (dynamo servic) - but it's just a guess. In general I hope we can either:
- use a fallback and avoid referencing binaries that require windows
- leverage ASC on linux without using the windows registry ( only makes sense after ASC actually supports linux)
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.
for example the linux github action build log is now full of:
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Utilities.cs(91,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.convert(double, string, string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Utilities.cs(104,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.convert(double, string, string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Quantity.cs(30,31): warning CA1416: This call site is reachable on all platforms. 'Quantity.getName()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Quantity.cs(24,33): warning CA1416: This call site is reachable on all platforms. 'Quantity.getTypeId()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Symbol.cs(20,40): warning CA1416: This call site is reachable on all platforms. 'Symbol.getPrefixOrSuffix()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Unit.cs(24,31): warning CA1416: This call site is reachable on all platforms. 'Unit.getName()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Unit.cs(30,33): warning CA1416: This call site is reachable on all platforms. 'Unit.getTypeId()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Unit.cs(40,61): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.getConvertibleUnits(string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Unit.cs(53,34): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.getQuantitiesContainingUnit(string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Symbol.cs(27,33): warning CA1416: This call site is reachable on all platforms. 'Symbol.getTypeId()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Symbol.cs(33,38): warning CA1416: This call site is reachable on all platforms. 'Symbol.getUnit()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Utilities.cs(116,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.parse(string, string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Utilities.cs(128,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.parse(string, string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Utilities.cs(139,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.parseUnitless(string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Symbol.cs(45,61): warning CA1416: This call site is reachable on all platforms. 'PrefixOrSuffix.hasSpace()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Symbol.cs(39,111): warning CA1416: This call site is reachable on all platforms. 'PrefixOrSuffix.getText()' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Unit.cs(93,20): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.areUnitsConvertible(string, string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Quantity.cs(59,37): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.getQuantity(string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/Quantity.cs(69,45): warning CA1416: This call site is reachable on all platforms. 'UnitsEngine.getQuantity(string)' is only supported on: 'Windows' 7.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [/home/runner/work/Dynamo/Dynamo/Dynamo/src/Libraries/DynamoUnits/UnitsCore.csproj]
I don't know if these were here in previous builds - we should probably make these errors.
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.
Great observation, @mjkkirschner -- you’re so right that core components like the units library should remain platform-independent. I hadn’t considered that angle after being away from the project for years, but it’s an important point I’ll keep in mind moving forward.
On Linux ASC support, my research shows it’s still limited: I found only a Linux decision log from last year and a work-breakdown document from a few months ago. From that, it looks like Linux support isn’t production-ready yet. With that in mind, I’ve created an epic to refactor ASC detection in a platform-agnostic way once Linux support becomes available -- you can review the planned tasks in this epic.
For the current PR, I’ve taken a reflection-based approach to call the ASC wrapper APIs for discovering installed ASC paths. On non-Windows platforms, the code falls back to the existing behavior of loading schemas from the bundled location. This keeps cross-platform compatibility while preserving full Windows functionality.
As for the CA1416 compilation warnings, I initially thought they were due to my changes, but they also appear on the master branch. They originate from the ForgeUnits.UnitsEngine APIs. I’m not sure whether this will cause issues for Linux-based Dynamo core, but it’s been present for a while now.
Would you be able to give this another review when you have a chance? I’d appreciate your approval so we can move this forward into the next phase. Thanks again for the thoughtful feedback!
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.
@benglin After a discussion on similar topic on slack with Mike I filed a task for this improvement, https://jira.autodesk.com/browse/DYN-9606
| /// </summary> | ||
| internal static void Initialize() | ||
| { | ||
| var assemblyFilePath = Assembly.GetExecutingAssembly().Location; |
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.
The Initialize method now builds a list of candidate schema directories, giving ASC-based directories the highest priority. Detection of ASC directories is handled entirely in AddAscSchemaPaths. If schemas from an ASC directory load successfully, that directory is used and no further lookup is performed.
|
|
||
| return false; | ||
| } | ||
|
|
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.
As shown here, AddAscSchemaPaths uses reflection to obtain AscSdkWrapper from DynamoInstallDetective.dll. This approach avoids introducing a build-time dependency on the DLL, which is Windows-only, while DynamoUnits remains platform-agnostic.
| // or ASC might not be installed on the system. | ||
| } | ||
| } | ||
|
|
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.
ForgeUnits.SchemaUtility.addDefinitionsFromFolder will succeed even if the specified schemaDirectory is empty. Therefore, it’s important to verify that required subdirectories such as quantity, symbol, and others exist under the unit directory; otherwise, the directory won’t qualify as a valid source for ForgeUnits.UnitsEngine.
| @@ -1,4 +1,4 @@ | |||
| using DynamoUnits; | |||
| using DynamoUnits; | |||
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.
The ImportHelpers.CalculateMillimeterPerUnit method was previously hard-coded to create a Unit object using autodesk.unit.unit:millimeters-1.0.1. Although this schema version still exists in v2, the latest version is 2.0.0, which is what appears in the dynamoUnit.ConvertibleUnits list. As a result, the original ConvertibleUnits.Contains(mm) failed since 1.0.1 != 2.0.0. To address this, the comparison was updated to use Unit.Name values instead.
| [Test, Category("UnitTests")] | ||
| public void CanCreateForgeUnitType_FromLoadedTypeString() | ||
| { | ||
| // Exact version exists: use the version as-specified |
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.
The test environment now includes 2.0.0 schemas, so the test cases were updated accordingly. For version-based tests, we replaced 1.0.2 with 99.9.9 to represent a "future version," since 99.9.9 is guaranteed not to exist and thus serves as a reliable placeholder. For the same reason, we use 0.0.1 as the "past version," ensuring it also doesn’t exist.
| private MeasurementInputBaseConcrete measurementInputBase; | ||
| private AssemblyHelper assemblyHelper; | ||
|
|
||
|
|
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.
The test cases were updated to compare only type names (e.g., "autodesk.unit.quantity:force") instead of full type IDs (e.g., "autodesk.unit.quantity:force-1.0.2"), making the tests less sensitive to schema version changes.
mjkkirschner
left a comment
mjkkirschner
left a comment
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.
nice!
Purpose
This PR enhances the
DynamoUnitsschema loading system to automatically discover and use Autodesk Shared Components (ASC) schema installations as alternative schema sources. Previously, the system only used the bundled schemas (v1.0.4) that ship with Dynamo. This change implements a candidate directory approach that prioritizes ASC schemas from newer product releases when available.The enhancement allows Dynamo to automatically use unit schemas from ASC installations (2026, 2027, 2028, etc.) found on the system, falling back to the bundled
v1.0.4schemas only when no ASC schemas are available. This ensures users get access to potentially newer schema definitions when newer Autodesk products are installed.Scenario 1: Implicit Version Selection
After restarting Dynamo with ASC schemas available, the graph is automatically upgraded from schema version
1.x.xto2.x.xwithout any manual changes. Conversely, if a graph saved with2.x.xschemas is opened in an environment where only1.x.xschemas are available, it will be automatically downgraded to1.x.xwithout user intervention.With bundled Dynamo schemas
With ASC schemas made available
Scenario 2: Explicit Version Selection
Even when the user explicitly hard-codes the unit schema
typeIdand creates aUnitobject based on thattypeId, the system will use the requested version if it is available. If the requested version is not found, it will automatically fall back to the next best available version on the user's machine. For example, if the user specifiestypeIdfor version1.x.x, the system will not automatically pick2.x.xeven if it is present. However, if the user requests2.x.xand only1.x.xis available, the system will auto-downgrade to1.x.x. The following images show this in action.Graph on Dynamo with
1.x.xschemasWhen a
2.x.xschema is explicitly requested but unavailable, the system automatically falls back to the available1.x.xschema.Graph on Dynamo with
2.x.xschemasWhen a
1.x.xschema is explicitly requested, the system continues to use1.x.xeven if2.x.xschemas became available.Declarations
Check these if you believe they are true
Release Notes
Enhanced
DynamoUnitsschema loading to automatically discover and use Autodesk Shared Components schema installations as alternative sources, with fallback to bundledv1.0.4schemas, providing access to potentially newer unit definitions when Autodesk products are installed.Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
Background for Reviewers:
This change enhances the schema loading system to leverage available ASC installations. The key architectural decisions made:
Candidate Directory Approach: Instead of using only bundled schemas, the system now tries multiple candidate directories in priority order (config path → ASC installations from newer product releases → bundled
v1.0.4schemas).Schema Validation: Added
IsValidSchemaDirectory()to verify that discovered directories contain the required subdirectories (dimension,quantity,symbol,unit) before attempting to load schemas.ASC Integration: Uses
AscSdkWrapperto discover installed ASC components via Windows registry. Required addingInternalsVisibleTo("DynamoUnits")toDynamoInstallDetectivebecause the publicInstalledAscLookUpclass doesn't expose the version enumeration method we needed.Error Reporting: Enhanced error messages to show all attempted paths rather than just a generic failure message.
Version Prioritization: ASC versions are now sorted newest-first at the source (
AscSdkWrapper.GetMajorVersions()) to ensure latest schemas are tried before older ones.The changes maintain backward compatibility while significantly improving robustness in environments where multiple Autodesk products are installed.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of