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

feat: Create an environment variable override for mayapy executable #154

Open
wants to merge 2 commits into
base: mainline
Choose a base branch
from

Conversation

Ahuge
Copy link

@Ahuge Ahuge commented May 23, 2024

NOTE: The MayaAdaptor expects that the MayaPY executable is named mayapy and is set on the PATH. If this is not the case, you can set the MAYA_ADAPTOR_MAYAPY_EXECUTABLE environment variable to the path to the MayaPy executable.

What was the problem/requirement? (What/Why)

If mayapy isn't in the default PATH, mayapy cannot be found

What was the solution? (How)

We have implemented an environment variable solution like was done for deadline-cloud-for-nuke.

What is the impact of this change?

Additional feature support for a new MAYA_ADAPTOR_MAYAPY_EXECUTABLE environment variable to point to a specific mayapy path.

How was this change tested?

Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.

Required: paste the contents of job_bundle_output_tests/test-job-bundle-results.txt here

Was this change documented?

Is this a breaking change?


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Ahuge Ahuge requested a review from a team as a code owner May 23, 2024 19:37
NOTE: The MayaAdaptor expects that the MayaPY executable is named `mayapy` and is set on the PATH. If this is not the case, you can set the `MAYA_ADAPTOR_MAYAPY_EXECUTABLE` environment variable to the path to the MayaPy executable.
Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
@Ahuge Ahuge force-pushed the ah/feature/mayapy-adaptor-exe-environment-variable-override branch from 3b33136 to 9fd213f Compare May 23, 2024 19:39
Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
@Ahuge Ahuge force-pushed the ah/feature/mayapy-adaptor-exe-environment-variable-override branch from 072c76c to a0042e5 Compare May 23, 2024 19:45
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@crowecawcaw crowecawcaw left a comment

Choose a reason for hiding this comment

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

Very useful, thank you!

As a nit, since this path is more a usage feature than a development one, could you move the docs folder instead? I think we should create an FAQ.md file in there for now, similar to this one for the Houdini submitter.

@@ -362,7 +362,7 @@ def _start_maya_client(self) -> None:
Raises:
FileNotFoundError: If the maya_client.py file could not be found.
"""
mayapy_exe = "mayapy"
mayapy_exe = os.environ.get("MAYA_ADAPTOR_MAYAPY_EXECUTABLE", "mayapy")
Copy link
Contributor

Choose a reason for hiding this comment

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

The concept we have is that an environment defined in the queue or the job template should ensure that the correct mayapy is in the PATH when this adaptor runs. Do you think that will work in the cases you're planning for, or do you have more details about how an environment variable override is better?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Mark,

I was using the Queue Environment to set the MAYA_ADAPTOR_MAYAPY_EXECUTABLE variable myself.

I understand that your reasoning for expecting "mayapy" to just exist in the path makes sense, especially in a conda/rez environment system.

For my customers and setup, we already have a system to set environment variables and expose required environment variables to the customer.

From our opinion, environment variables allow for a more flexible, user friendly approach to customization compared to writing custom scripts to add arbitrary directories to a path.
The environment variable approach allows specific targeted configuration without requiring custom path modification scripts to be written or an integration with conda/rez which would likely require developers as well.

I spoke separately with Pauline about this and totally understand your reasoning for not wanting this included.
It sounds like the deadline-cloud-for-nuke code that I referenced for this was mistakenly left in from a development phase.

I'll put in my last request for this and if your team decided that it is not something you would like to support, I understand.
I feel that this environment variable would allow both methods (adding to path or overriding with environment variables) and depending whichever the client finds easier to manage for themselves they can take advantage of.
I think the reason that I feel strongly about environment variables (as you've probably seen across all of my PRs) is that environment variables are within the realm of things that a non-technical, or lightly technical customer can get their head around, whereas adding to a path, or custom coding now requires a developer in most cases.

I appreciate you spending the time to read this and considering the PR.

Thanks
-Alex

Copy link
Contributor

@mwiebe mwiebe Jun 3, 2024

Choose a reason for hiding this comment

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

For my customers and setup, we already have a system to set environment variables and expose required environment variables to the customer.

This is great input, and is the kind of data we need to use in deciding how to structure things.

I feel that this environment variable would allow both methods (adding to path or overriding with environment variables) and depending whichever the client finds easier to manage for themselves they can take advantage of.
I think the reason that I feel strongly about environment variables (as you've probably seen across all of my PRs) is that environment variables are within the realm of things that a non-technical, or lightly technical customer can get their head around, whereas adding to a path, or custom coding now requires a developer in most cases.

This makes a lot of sense. Within a queue environment, you can add a new environment variable very easily by adding to the variables field, but to modify the PATH with a new item requires switching to code. I think we'll be able to build up better support materials to simplify usage of conda/rez, but an escape hatch that's simple to apply is always useful.

With that said, I wonder if we can simplify down the environment variable name? Telling applications which mayapy binary to use doesn't seem specific to this adaptor, so if there is precedent for a name like MAYAPY_EXECUTABLE or similar, I think that would be preferable. While an internet search doesn't find that, it does find MAYA_EXECUTABLE in https://github.com/nerdvegas/vfxcmake/blob/master/cmake/FindMaya.cmake. MAYA_LOCATION also features there. Some possibilities for the environment variable here could be:

  1. If MAYAPY_EXECUTABLE exists, use it as the mayapy binary path.
  2. If MAYA_LOCATION exists, use it, with bin/mayapy joined to the end.
  3. If MAYA_EXECUTABLE exists, swap out its binary filename for mayapy.

Probably the first option is the most straightforward, and closest to what you've written in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants