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

Feature: RQD use host env vars #1324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tilix4
Copy link

@Tilix4 Tilix4 commented Oct 20, 2023

Link the Issue(s) this Pull Request is related to.
Mail list discussion

Summarize your change.
Based on RQD_USE_PATH_ENV_VAR but generalized to any host env var.

def _createCommandFile(self, command):
"""Creates a file that subprocess. Popen then executes.
@type command: string
@param command: The command specified in the runFrame request
@rtype: string
@return: Command file location"""
# TODO: this should use tempfile to create the files and clean them up afterwards
# TODO: this should use tempfile to create the files and clean them up afterwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental change?

Copy link
Author

Choose a reason for hiding this comment

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

I think so

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the trailing spaces.

@@ -70,6 +70,9 @@
# Use the PATH environment variable from the RQD host.
RQD_USE_PATH_ENV_VAR = False

# Use the environment variables from the RQD host.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this requires a more detailed explanation. In the future when we don't have the scope in which this was implemented, it will be hard to understand what this is about.

Beside that, your implementation is too broad, it could catch any envvar that contains PATH, when what you really want is to change PATH and PYTHONPATH.

Since there's already a option for PATH (RQD_USE_PATH_ENV_VAR), I suggest you add an option specifically for PYTHONPATH -> RQD_USE_PYTHONPATH_ENV_VAR.

Copy link
Author

Choose a reason for hiding this comment

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

I understand your point and it was one concern I had.
In fact, the need is wider than only changing PYTHONPATH. I need to set env vars when starting up the RQD host, for example for a root path like MYPROJ_RENDER_ROOT_PATH which would be set different depending on the artist machine (Z: for a windows, /mnt/myproj/renders for a linux...). That's why I've proposed something that broad, because able to deal with all env var use cases.

I could add a dedicated RQD_USE_PYTHONPATH_ENV_VAR if it is better to split, but we should discuss about a way to set any env var we like when starting up a RQD host.

Copy link
Author

Choose a reason for hiding this comment

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

I've studied the change proposed in this PR #1270, and it is a good proposal, but not a full solution for my use case. The env vars I'd like to set to the RQD process from the host must be set on the fly, depending on the list of projects the artist has access to for example, I don't want to keep the config file up-to-date for each computer.
The solution I see could be to add an argument --host-env-vars MYENVVAR MYPROJ_RENDER_ROOT_PATH ... to list the env vars I want the RQD process to load from the host.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get your point, and I think both the use case handled by #1270 and this PR are valid and could co-exist.

My suggestion is that we change the constant name on this PR to RQD_COPY_ALL_HOST_ENV_VARS or something similar to avoid confusion with RQD_HOST_ENV_VARS defined on #1270 .

Besides that, please add the new option to the rqd.conf example file.

Comment on lines +126 to +131
if rqd.rqconstants.RQD_USE_HOST_ENV_VARS:
for key, value in os.environ.items():
if "PATH" in key and key in self.frameEnv:
self.frameEnv[key] += os.pathsep + value

self.frameEnv[key] = value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change the logic to only catch PYTHONPATH

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be conflicting with the feature introduced in this PR (still in review) :
#1270

Copy link
Collaborator

@DiegoTavares DiegoTavares left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait to get this moving again.

@@ -70,6 +70,9 @@
# Use the PATH environment variable from the RQD host.
RQD_USE_PATH_ENV_VAR = False

# Use the environment variables from the RQD host.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get your point, and I think both the use case handled by #1270 and this PR are valid and could co-exist.

My suggestion is that we change the constant name on this PR to RQD_COPY_ALL_HOST_ENV_VARS or something similar to avoid confusion with RQD_HOST_ENV_VARS defined on #1270 .

Besides that, please add the new option to the rqd.conf example file.

def _createCommandFile(self, command):
"""Creates a file that subprocess. Popen then executes.
@type command: string
@param command: The command specified in the runFrame request
@rtype: string
@return: Command file location"""
# TODO: this should use tempfile to create the files and clean them up afterwards
# TODO: this should use tempfile to create the files and clean them up afterwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the trailing spaces.

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