Skip to content

Fix generated Spark config placement in raydp-submit - #491

Open
my-vegetable-has-exploded wants to merge 1 commit into
ray-project:masterfrom
my-vegetable-has-exploded:codex/fix-submit-conf-order
Open

my-vegetable-has-exploded wants to merge 1 commit into
ray-project:masterfrom
my-vegetable-has-exploded:codex/fix-submit-conf-order

Conversation

@my-vegetable-has-exploded

Copy link
Copy Markdown
Contributor

What does this PR do?

Prepend generated Spark --conf options before user arguments in raydp-submit, preserving application argument boundaries. Quote the Ray config path and add launcher regression tests. Explicit user --conf values now override generated defaults.

Why is this change needed?

The launcher inserts generated configs before the last argument, assuming it is the application resource. With application arguments, the configs instead reach the application parser.

For example:

raydp-submit --ray-conf /root/ray.conf main.py --parameter_file=tmp_param.json

Before (simplified SparkSubmit arguments):

main.py --conf spark.ray.log4j.config.file.name=log4j2.xml --conf spark.driver.host=<ip> --conf spark.driver.bindAddress=<ip> --parameter_file=tmp_param.json

This causes main.py: error: unrecognized arguments: --conf ....

After:

--conf spark.ray.log4j.config.file.name=log4j2.xml --conf spark.driver.host=<ip> --conf spark.driver.bindAddress=<ip> main.py --parameter_file=tmp_param.json

Related Issue

None.

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Test update
  • Refactoring
  • Build / CI / dependency change
  • Other

How was this tested?

python3 -m unittest discover -s tests -p test_raydp_submit.py -v
bash -n bin/raydp-submit
git diff --check

Tests fail on the original launcher and pass with the fix. They exercise the real shell launcher without a cluster, covering Python/JAR entrypoints, zero/multiple application arguments, spaces and empty values, --ray-conf, and explicit driver address precedence.

Checklist

  • I have added or updated tests if needed
  • I have updated documentation if needed
  • I have run relevant tests locally
  • I have checked that this change is backward compatible

No CLI changes. The intentional precedence change for duplicate --conf values is noted above; no separate documentation update is needed.

Prepend generated configs before user arguments to prevent them from reaching application parsers. Preserve spaces in the Ray config path and cover argument boundaries and explicit config precedence with launcher tests.

Signed-off-by: my-vegetable-has-exploded <wy1109468038@gmail.com>
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.

1 participant