Skip to content

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Nov 4, 2021

@DavidGOrtega DavidGOrtega temporarily deployed to internal November 4, 2021 09:47 Inactive
@0x2b3bfa0
Copy link
Member

@DavidGOrtega, I believe that $PATH is mandatory for runners to work. 🤔

@casperdcl
Copy link
Contributor

casperdcl commented Nov 4, 2021

The problem with whitelist-only is there are a few other things that a user would likely want preserved too...

  • NVIDIA_VISIBLE_DEVICES
  • CUDA_VISIBLE_DEVICES
  • LANG

I'd prefer blacklisting CML config vars instead (AWS_*ACCESS_KEY etc)

(vis. #721 (comment))

@casperdcl casperdcl mentioned this pull request Nov 4, 2021
7 tasks
@casperdcl casperdcl added cml-runner Subcommand discussion Waiting for team decision p1-important High priority security Sensitive flaws labels Nov 4, 2021
@dacbd
Copy link
Contributor

dacbd commented Nov 4, 2021

Consider using systemd-run to start the run.sh ? Should scrub vars set in the session with export but maintain system controlled ones like PATH

@dacbd
Copy link
Contributor

dacbd commented Nov 4, 2021

Consider using systemd-run to start the run.sh ? Should scrub vars set in the session with export but maintain system controlled ones like PATH

Ehh don't think this plays nice when executed in a docker container, so probably not the best

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Nov 4, 2021

Im checking. It's happening something interesting.
I was counting with spawn( {shell:true, env: {} }) which should create its own shell with system vars being expanded by the node ones. Funny is that in Macos seems to be fine... however inside a linux container Its not very happy...

@DavidGOrtega DavidGOrtega marked this pull request as draft November 9, 2021 19:46
@dacbd
Copy link
Contributor

dacbd commented Nov 20, 2021

Confirm that this is really only an issue for cloud-based runners; the fix might be in tpi?

@casperdcl
Copy link
Contributor

I have this issue on on-prem as well as cloud...

@tasdomas
Copy link
Contributor

This branch is now almost a year old. Should we make a decision on this?

@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 10 times, most recently from ad4e8cc to cce3788 Compare September 12, 2022 19:17
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 15:09 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 18:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 18:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 14, 2023 13:06 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 requested a review from dacbd February 14, 2023 13:25
@0x2b3bfa0 0x2b3bfa0 removed the blocked Dependent on something else label Feb 14, 2023
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 14, 2023 15:48 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:09 — with GitHub Actions Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:09 — with GitHub Actions Inactive
@0x2b3bfa0 0x2b3bfa0 enabled auto-merge (squash) February 21, 2023 17:10
@github-actions

This comment was marked as outdated.

1 similar comment
@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:14 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 disabled auto-merge May 9, 2024 22:04
@0x2b3bfa0 0x2b3bfa0 merged commit e7d27a5 into master May 9, 2024
@0x2b3bfa0 0x2b3bfa0 deleted the no-envars branch May 9, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-runner Subcommand discussion Waiting for team decision p1-important High priority security Sensitive flaws
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants