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

Specifying Julia executable/args via RuntimeEnv #116

Merged
merged 8 commits into from
Sep 15, 2023

Conversation

omus
Copy link
Member

@omus omus commented Sep 13, 2023

Depends on: beacon-biosignals/ray#8

Closes: #103, #100

TODO:

  • Add tests

@omus omus requested a review from kleinschmidt September 13, 2023 21:13
@omus omus self-assigned this Sep 13, 2023
@omus
Copy link
Member Author

omus commented Sep 13, 2023

I will validate that #100 is actually addressed locally tomorrow. I did do a test to ensure that --code-coverage is being passed through but haven't done a full end-to-end test.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #116 (af0bfed) into main (03fda94) will increase coverage by 20.63%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #116       +/-   ##
===========================================
+ Coverage   74.63%   95.26%   +20.63%     
===========================================
  Files           8        8               
  Lines         406      380       -26     
===========================================
+ Hits          303      362       +59     
+ Misses        103       18       -85     
Flag Coverage Δ
Ray.jl 95.26% <100.00%> (+20.63%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/runtime_env.jl 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@omus
Copy link
Member Author

omus commented Sep 13, 2023

Things for tomorrow:

  • Allow user specified Julia executable? Remote nodes may not have Julia installed at the same location
  • BASH injection vulnerability with Ray's runtime env code?
  • Specify additional default flags? e.g. --startup-file=no
  • Combine executable and args?
  • Deprecate py_executable
  • Test code coverage from drivers and workers don't overwrite coverage files
  • Client side tests

@omus
Copy link
Member Author

omus commented Sep 14, 2023

Allow user specified Julia executable? Remote nodes may not have Julia installed at the same location

Distributed.jl does allow end users to specify the path to the Julia executable. In Ray.jl specifying the Julia executable path would only really allow for having the driver and worker Julia paths to differ but all workers would need to be the same. Possibly specifying a Ray task with resource requirements could allow further heterogeneity. I'll punt on allowing user specified values for now here but it's an easy future addition if we need it later.

BASH injection vulnerability with Ray's runtime env code

In evaluating the original ray-2.5.1 code the only two parameters that are user definable are env_vars and java_jars. All other variables can not be specified via a passed in runtime environment. Since env_vars only set values in os.environ this cannot result in arbitrary code execution. It may be possible using java_jars to inject a carefully executed bash script which contained no spaces (e.g. {touch,file}) as the only escaping of user specified java_jars is handled by replacing spaces with escaped spaces. I have not checked the Java front end but if they validate the contents of java_jars before hand the threat can be mitigated.

Our update to context.py where we allow the client to specify how worker processes are spawned may be problematic if the Ray devs are highly concerned about this problem. Without this kind of support we may be limited to only launching the the Julia process on the path named julia.

Specify additional default flags? e.g. --startup-file=no

Any flags that julia_cmd() passes through will be passed in automatically. We can always add additional flags in later if we want them.

Combine executable and args?

I decided this was a good idea.

Deprecate py_executable

@kleinschmidt brought up that the same RuntimeEnvContext may be used to launch workers using different languages. Because of this we should leave py_executable as is

Test code coverage from drivers and workers don't overwrite coverage files

It works! Each coverage file includes the PID in the file name. This means our workers and driver coverage files aren't clobbering each other.

Client side tests

Done

@omus omus marked this pull request as ready for review September 14, 2023 20:31
Comment on lines +25 to 31
code = "using $(@__MODULE__); start_worker()"
cmd = `$(Base.julia_cmd()) -e $code`

# The keys of `context` must match what is supported by the Python `RuntimeEnvContext`:
# https://github.com/ray-project/ray/blob/ray-2.5.1/python/ray/_private/runtime_env/context.py#L20-L45
context = Dict("env_vars" => env_vars)
context = Dict("julia_command" => cmd.exec, "env_vars" => env_vars)

Copy link
Member

Choose a reason for hiding this comment

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

just to make sure I understand: this works to enable coverage because Base.julia_cmd() includes all the flags that were used to call the current process?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your understanding is correct

@kleinschmidt kleinschmidt merged commit a70df97 into main Sep 15, 2023
4 checks passed
@kleinschmidt kleinschmidt deleted the cv/driver-controlled-julia-cmd branch September 15, 2023 18:42
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.

Allow Ray.jl driver more control on spawning worker processes
2 participants