Skip to content

Conversation

@MustafaJafar
Copy link
Member

@MustafaJafar MustafaJafar commented Aug 25, 2025

Changelog Description

Remove CollectUSDPinningEnvVars plugin.

Additional Info

This is tested with ynput/ayon-usd#93
This is part of solution for ynput/ayon-usd#92

Testing notes:

  1. everything should work as before.

@MustafaJafar MustafaJafar requested a review from BigRoy August 25, 2025 17:35
@MustafaJafar MustafaJafar self-assigned this Aug 25, 2025
@MustafaJafar MustafaJafar added the type: maintenance Changes to the code that don't affect product functionality (Technical debt, refactors etc.)) label Aug 25, 2025
@MustafaJafar MustafaJafar marked this pull request as draft August 25, 2025 17:35
@MustafaJafar
Copy link
Member Author

This PR is made to reflect the changes I'm doing on my side when testing USD publish plugins.

@MustafaJafar MustafaJafar marked this pull request as ready for review October 24, 2025 12:42
@kalisp
Copy link
Member

kalisp commented Nov 11, 2025

If I understand it correctly this moves collect_usd_pinning_env_vars.py‎ to ayon-usd, correct? What will happen if the plugin will stay here and be also in ayon-usd? (Or wont be there.)
The point is if there should be some dependency (and therefore one of the PRs should wait for release of the other one).
(As if we remove plugin from here AND customer will not update ayon-usd, environments will be missing.)

Actually, are these environment necessary for ayon-usd or are they necessary only farm (in that case they should probably stay here. As there are other DCC based plugins, which are in ayon-deadline as they make sense only there. If you dont have Deadline you dont need them in your DCC.

@MustafaJafar
Copy link
Member Author

MustafaJafar commented Nov 11, 2025

If I understand it correctly this moves collect_usd_pinning_env_vars.py‎ to ayon-usd, correct?

yes.

What will happen if the plugin will stay here and be also in ayon-usd? (Or wont be there.) The point is if there should be some dependency (and therefore one of the PRs should wait for release of the other one). (As if we remove plugin from here AND customer will not update ayon-usd, environments will be missing.)

This plugin is related to pinning feature in our USD Resolver. It updates the job submission vars.
tbh, it doesn't work as expected and it requires fixes in the usd addon and the resolver itself. we can also argue this plugin can be removed without adding any dependency as it doesn't just work.

Actually, are these environment necessary for ayon-usd or are they necessary only farm (in that case they should probably stay here. As there are other DCC based plugins, which are in ayon-deadline as they make sense only there. If you dont have Deadline you dont need them in your DCC.

Tbh, I'm not sure about the best approach here. I was doing some fixes and I recall the todo in this plugin was mentioned couple of times. so I included it among those fixes.

Tagging @BigRoy @antirotor for further info.

@MustafaJafar
Copy link
Member Author

After some brainstorming and testing ynput/ayon-usd#93 I think we no longer need CollectUSDPinningEnvVars collector. Actually, I removed it from the linked PR as well.

@BigRoy
Copy link
Contributor

BigRoy commented Dec 2, 2025

@MustafaJafar can you elaborate on why you think it may not be needed anymore?

Without these vars set on the job:

{
        "PINNING_FILE_PATH": usd_pinning_rootless_file_path,
        "ENABLE_STATIC_GLOBAL_CACHE": "1",
}

What would be setting the pinning file for the Deadline job then?

Or did you just mean that you moved that logic, e.g. here?

@MustafaJafar
Copy link
Member Author

What would be setting the pinning file for the Deadline job then?

Or did you just mean that you moved that logic, e.g. here?

yes, I've moved that logic to the extractor. there are also further info mentioned in ynput/ayon-usd#93 (comment)

Copy link
Contributor

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes seem fine if ayon-usd logic will be taking this over.
I'm not sure if this needs an ayon_compatible requirement to ayon-usd to ensure it's only valid with an ayon usd version that has the logic added.

Yet at the same time, I've heard a few times "this is broken" anyway so I'm not sure how strongly we should hold to this for backwards compatibility. I'll leave that up to @MustafaJafar and @antirotor to decide since I think you've been discussing this mostly. I'm fine either way as long as we don't cut off anyone on this feature for a long time.

@MustafaJafar
Copy link
Member Author

I don't have a strong opinion tbh.
But as said it's broken anyway. So, I'd vote for silently skipping adding a compatibility.

@iLLiCiTiT iLLiCiTiT added type: enhancement Improvement of existing functionality or minor addition and removed type: maintenance Changes to the code that don't affect product functionality (Technical debt, refactors etc.)) labels Dec 12, 2025
Copy link
Member

@iLLiCiTiT iLLiCiTiT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at get_usd_pinning_envs it really doesn't make sense to have it here.

@iLLiCiTiT iLLiCiTiT merged commit 651cefb into develop Dec 12, 2025
1 check passed
@iLLiCiTiT iLLiCiTiT deleted the bugfix/92-clean-up-usd-publish-plugins branch December 12, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement Improvement of existing functionality or minor addition

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants