Correctly assign asset type in AssetProfile #47659
Merged
+118
−41
Conversation
This file contains 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
uranusjr
requested review from
ephraimbuddy,
pierrejeambrun,
XD-DENG and
ashb
as code owners
Lee-W
approved these changes
Mar 12, 2025
uranusjr
force-pushed
the
asset-type-refactor
branch
from
818b350 to
5f249e0
Compare
The asset_type field in AssetProfile is for distinguishing between different asset-related types (Asset, ref, or alias). However, it used type(obj).__name__, which does not account for subclasses correctly. Since we don't really care about asset subclasses in the scheduler (they are only meaningful to the user, either in UI and at runtime in the task runner), we can simply coerce them into the base class we care about. This means ditching the type() call altogether to just use the base class directly. The field is also renamed to 'type' to avoid confusion since 'asset_type' has another meaning in the Asset class. Two tests on asset ref around this has been marked as xfail since the task runner does not currently handle them correctly. They will be fixed in an upcoming PR.
uranusjr
force-pushed
the
asset-type-refactor
branch
from
5f249e0 to
00c6372
Compare
|
One provider test failing, but I’m seeing the same in other PRs too. Merging this since it seems entirely unrelated. |
This was referenced Mar 13, 2025
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.
The asset_type field in AssetProfile is for distinguishing between different asset-related types (Asset, ref, or alias). However, it used type(obj).name, which does not account for subclasses correctly. Since we don't really care about asset subclasses in the scheduler (they are only meaningful to the user, either in UI and at runtime in the task runner), we can simply coerce them into the base class we care about. This means ditching the type() call altogether to just use the base class directly.
The field is also renamed to 'type' to avoid confusion since 'asset_type' has another meaning in the Asset class.
Two tests on asset ref around this has been marked as xfail since the task runner does not currently handle them correctly. They will be fixed in an upcoming PR.
Close #47598.