Fix docker-compose bash commands multiline parsing#18922
Fix docker-compose bash commands multiline parsing#18922harupy merged 3 commits intomlflow:masterfrom UnfixedMold:bug/18899-docker-compose-multiline-bash-commands
Conversation
|
@UnfixedMold Thank you for the contribution! Could you fix the following issue(s)? ⚠ DCO checkThe DCO check failed. Please sign off your commit(s) by following the instructions here. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md#sign-your-work for more details. |
| command: > | ||
| /bin/bash -c " | ||
| pip install --no-cache-dir psycopg2-binary boto3 && | ||
| mlflow server \ |
There was a problem hiding this comment.
We can't just remove the \ because the args would be treated as separate shell commands. We could put the entire mlflow server command on one line, thus removing \, but that becomes too long IMO and I haven't checked the repo’s linting/hooks configuration. So I kept the safer multi-line version with \.
If maintainers prefer the inline form, I can change it.
There was a problem hiding this comment.
I double checked. Just removing the \ doesn't fix the issue and just leads to the same behavior as right now - cmd args not getting parsed and and sole mlflow server (no args) being run in the container
For a bare minimal fix, we could just change the " to ' (like it's originally in create-bucket entrypoint) and that would work, but idk how stable this is, I can't even tell why ' parsing works, but " doesn't
Escaping env vars would still need to be adressed no matter the solution, it's a separate bug, that I've noticed while working on this one.
There was a problem hiding this comment.
Sorry I confused > and |, yea it seems the pipe doesn't concatenate the lines into a single command. Thanks for testing.
|
can you fix the lint error? |
|
Yeah sure, I'll address the lint errors |
|
looks like unrelated commits are included in this PR. Can you remove them? |
|
@harupy its just the sync from master, since master got updated while i was working on this PR. Now branch is synched with master but contains traces of the sync (unrelated commits). Should I still remove them? |
|
Yes, otherwise Copilot and me will be Included as co-author of this PR |
- switched from shell form to exec array form - added quotes around env vars for safe expansion Signed-off-by: Henrikas Antanas <henrikasgir@gmail.com>
Signed-off-by: Henrikas Antanas <henrikasgir@gmail.com>
Signed-off-by: Henrikas Antanas <henrikasgir@gmail.com>
|
@harupy fixed. Cherry picked only relevant commits. |
Signed-off-by: Henrikas Antanas <henrikasgir@gmail.com> Signed-off-by: Tian Lan <sky.blue266000@gmail.com>
🛠 DevTools 🛠
Install mlflow from this PR
For Databricks, use the following command:
Related Issues/PRs
Resolve #18899
What changes are proposed in this pull request?
Why?
Initially I noticed that the bash script was running
mlflow serverwithout any command-line arguments. The root issue was that backslashes were not parsed correctly when the command was wrapped in double quotes inside a folded YAML block. Switching to single quotes would have fixed it, but moving to the exec array form is a cleaner and more reliable solution. I also updated the bash script in thecreate-bucketservice for consistency and to avoid similar issues in the future (it worked previously only because it already used single quotes).I also switched
>to|so the script keeps its actual line breaks instead of being folded into one long string. This doesn’t change behavior in this case but is cleaner and avoids future YAML folding issues.Environment variables with special characters (e.g. passwords) were also breaking command parsing, so I added quotes around all env vars to make expansion safe.
How to test the fix
To see what the container is actually running, temporarily add
set -xat the top of the command block, for example:This is only for debugging and shouldn’t be left in production code, since it exposes secrets in the logs.
Then start the compose with
Then check the logs with
You should see the real expanded commands prefixed with +, for example:
How is this PR tested?
Does this PR require documentation update?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/tracking: Tracking Service, tracking client APIs, autologgingarea/models: MLmodel format, model serialization/deserialization, flavorsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflowsarea/gateway: MLflow AI Gateway client APIs, server, and third-party integrationsarea/prompts: MLflow prompt engineering features, prompt templates, and prompt managementarea/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionalityarea/projects: MLproject format, project running backendsarea/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notesShould this PR be included in the next patch release?
Yesshould be selected for bug fixes, documentation updates, and other small changes.Noshould be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.What is a minor/patch release?
Bug fixes, doc updates and new features usually go into minor releases.
Bug fixes and doc updates usually go into patch releases.