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

Fix timeout regex for spark submit command #4017

Conversation

timma-yelp
Copy link
Contributor

@timma-yelp timma-yelp commented Mar 4, 2025

Problem

We check if there is already a timeout command in the user's spark command before auto adding it, but the check doesn't work with the case that an CLI argument is used, e.g. timeout -v spark-submit

Solution

Fix the regex which checks if timeout is already present

Testing

I test ran the spark-submit locally on dev-box with timeout -v 1m, as expected it timed out at 1m
command and output logs can be found here

@timma-yelp timma-yelp requested review from nemacysts and chi-yelp March 4, 2025 15:05
@chi-yelp
Copy link
Contributor

chi-yelp commented Mar 4, 2025

Could you add an unit test and also add test run details (i.e. local paasta spark-run) in the PR description?

Example local spark-run cmd:

.tox/py38-linux/bin/paasta spark-run --aws-profile=dev --cmd "<timeout...> spark-submit /code/integration_tests/s3.py"

@timma-yelp
Copy link
Contributor Author

Could you add an unit test and also add test run details (i.e. local paasta spark-run) in the PR description?

Example local spark-run cmd:

.tox/py38-linux/bin/paasta spark-run --aws-profile=dev --cmd "<timeout...> spark-submit /code/integration_tests/s3.py"

I looked for existing unit tests to update them but didn't find any, is it ok to add new one?

will update the description with local spark-run command

@chi-yelp
Copy link
Contributor

chi-yelp commented Mar 4, 2025

Could you add an unit test and also add test run details (i.e. local paasta spark-run) in the PR description?
Example local spark-run cmd:

.tox/py38-linux/bin/paasta spark-run --aws-profile=dev --cmd "<timeout...> spark-submit /code/integration_tests/s3.py"

I looked for existing unit tests to update them but didn't find any, is it ok to add new one?

will update the description with local spark-run command

Sure, feel free to create one, thanks!

chi-yelp
chi-yelp previously approved these changes Mar 6, 2025
@timma-yelp timma-yelp merged commit 04964d5 into master Mar 6, 2025
10 checks passed
@timma-yelp timma-yelp deleted the timma/MLCOMPUTE-3674_fix_timeout_regex_for_spark_submit_command branch March 6, 2025 14:37
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.

2 participants