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.

Sign up for GitHub

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

Jump to bottom

Feature: RQD use host env vars #1324

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

Feature: RQD use host env vars #1324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rqd/rqd/rqconstants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

RQD_USE_HOST_ENV_VARS = False

RQD_BECOME_JOB_USER = True
RQD_CREATE_USER_IF_NOT_EXISTS = True
RQD_TAGS = ''
Expand Down Expand Up @@ -184,6 +187,8 @@
RQD_USE_IPV6_AS_HOSTNAME = config.getboolean(__section, "RQD_USE_IPV6_AS_HOSTNAME")
if config.has_option(__section, "RQD_USE_PATH_ENV_VAR"):
RQD_USE_PATH_ENV_VAR = config.getboolean(__section, "RQD_USE_PATH_ENV_VAR")
if config.has_option(__section, "RQD_USE_HOST_ENV_VARS"):
RQD_USE_HOST_ENV_VARS = config.getboolean(__section, "RQD_USE_HOST_ENV_VARS")
if config.has_option(__section, "RQD_BECOME_JOB_USER"):
RQD_BECOME_JOB_USER = config.getboolean(__section, "RQD_BECOME_JOB_USER")
if config.has_option(__section, "RQD_TAGS"):
Expand Down
11 changes: 10 additions & 1 deletion rqd/rqd/rqcore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,22 @@ def __createEnvVariables(self):
if 'GPU_LIST' in self.runFrame.attributes:
self.frameEnv['CUE_GPU_CORES'] = self.runFrame.attributes['GPU_LIST']

# Add host environment variables
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
Comment on lines +126 to +131
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



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.

try:
if platform.system() == "Windows":
rqd_tmp_dir = os.path.join(tempfile.gettempdir(), 'rqd')
Expand Down
Loading