Skip to content
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

Allow job environment variable to be specified via context or instance #32

Merged
merged 17 commits into from
Oct 8, 2024

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Sep 11, 2024

Changelog Description

Allow job environment variable to be specified via context or instance. This allows specific hosts, or other integrations to alter the behavior of what needs to be submitted along. And also de-duplicates a lot of list of environment variables across the different submitters.

Additional info

Testing notes:

  1. Deadline submissions should still work

@BigRoy BigRoy requested a review from antirotor September 11, 2024 22:17
@BigRoy BigRoy self-assigned this Sep 11, 2024
@BigRoy BigRoy added the type: enhancement Improvement of existing functionality or minor addition label Sep 11, 2024
@BigRoy
Copy link
Contributor Author

BigRoy commented Sep 11, 2024

For whatever reason the Celaction submitter has no env vars being published along - none here? @antirotor

@antirotor
Copy link
Member

For whatever reason the Celaction submitter has no env vars being published along - none here? @antirotor

Not sure why at all, this is playground of @jakubjezek001

BigRoy added 4 commits October 3, 2024 22:39
… enhancement/unify_env_var_process

# Conflicts:
#	client/ayon_deadline/plugins/publish/houdini/submit_houdini_cache_deadline.py
#	client/ayon_deadline/plugins/publish/houdini/submit_houdini_render_deadline.py
#	client/ayon_deadline/plugins/publish/maya/submit_maya_deadline.py
@BigRoy BigRoy requested a review from jakubjezek001 October 3, 2024 20:57
@BigRoy BigRoy marked this pull request as ready for review October 7, 2024 09:43
@kalisp
Copy link
Member

kalisp commented Oct 7, 2024

Do we want to pass PATH env var?
Tested this PR in AE and PATH is new addition
image

@BigRoy
Copy link
Contributor Author

BigRoy commented Oct 7, 2024

Do we want to pass PATH env var? Tested this PR in AE and PATH is new addition image

Preferably not - that's coming from the Nuke submission originally here.

I can for now move that to be Nuke-specific but preferably we can remove that completely. @antirotor @jakubjezek001 @moonyuet do you know?

For now made it backwards compatible here: 626930a by still submitting it along but only for Nuke submissions.

@moonyuet
Copy link
Member

moonyuet commented Oct 8, 2024

I can for now move that to be Nuke-specific but preferably we can remove that completely. @antirotor @jakubjezek001 @moonyuet do you know?

For now made it backwards compatible here: 626930a by still submitting it along but only for Nuke submissions.

I agree with @kalisp as it does not affect the render submission or rendering when I manually removed the environment variable and requeue the job for 3dsmax and maya

@BigRoy
Copy link
Contributor Author

BigRoy commented Oct 8, 2024

I agree with @kalisp as it does not affect the render submission or rendering when I manually removed the environment variable and requeue the job for 3dsmax and maya

Yes - thanks, it should be fixed with 626930a - can you confirm?

Preferably I'd also remove it for Nuke - but I have no clue why it was there before. We can easily remove that in a separate PR to make it a 'clear' change in the changelog. @jakubjezek001 any idea?

For whatever reason the Celaction submitter has no env vars being published along - none here?

Also @jakubjezek001 any ideas about this? I suppose it should get the env vars? If so, I can add it to the submitter if anyone is able to test it.

Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

Tested successfully with Maya and Max.
image

@jakubjezek001 jakubjezek001 added the community Issues and PRs coming from the community members label Oct 8, 2024
Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

Tested with CollectDeadlineNukeJobEnvVars and without, all works perfect! Lets remove the CollectDeadlineNukeJobEnvVars and keep this workflow unified with others.

Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

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

Tested AE, Nuke, Maya, seems fine.

@BigRoy
Copy link
Contributor Author

BigRoy commented Oct 8, 2024

For whatever reason the Celaction submitter has no env vars being published along - none here? @antirotor

Thanks everyone - only remaining piece is this question. What to do with the Celaction submitter. I suppose I should best just enforce that also passing along the env vars, right? (even if it didn't before?)

Is anyone able to test Celaction submissions?

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Oct 8, 2024

Isn't this against proposal in ynput/ayon-core#876 ?

Just asking...

@BigRoy
Copy link
Contributor Author

BigRoy commented Oct 8, 2024

Isn't this against proposal in ynput/ayon-core#876 ?

I'd say it's actually a first step to that issue. Why do you feel it's "against" it?

@BigRoy BigRoy requested a review from iLLiCiTiT October 8, 2024 10:54
@iLLiCiTiT
Copy link
Member

I'd say it's actually a first step to that issue. Why do you feel it's "against" it?

Because we will need to change all addons to match this structure, if we add more explicit keys for publish and render jobs in future, we will have to define new keys and keep backwards compatibility, and again update all addons. Maybe not directly against, just annoying handling of backwards/forwards compatibility across all addons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues and PRs coming from the community members type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants