Skip to content

Fix docker-compose bash commands multiline parsing#18922

Merged
harupy merged 3 commits intomlflow:masterfrom
UnfixedMold:bug/18899-docker-compose-multiline-bash-commands
Nov 19, 2025
Merged

Fix docker-compose bash commands multiline parsing#18922
harupy merged 3 commits intomlflow:masterfrom
UnfixedMold:bug/18899-docker-compose-multiline-bash-commands

Conversation

@UnfixedMold
Copy link
Contributor

@UnfixedMold UnfixedMold commented Nov 19, 2025

🛠 DevTools 🛠

Open in GitHub Codespaces

Install mlflow from this PR

# mlflow
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/18922/merge
# mlflow-skinny
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/18922/merge#subdirectory=libs/skinny

For Databricks, use the following command:

%sh curl -LsSf https://raw.githubusercontent.com/mlflow/mlflow/HEAD/dev/install-skinny.sh | sh -s pull/18922/merge

Related Issues/PRs

Resolve #18899

What changes are proposed in this pull request?

  • switched from shell form to exec array form
  • changed > to |
  • added quotes around env vars for safe expansion

Why?

Initially I noticed that the bash script was running mlflow server without 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 the create-bucket service 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 -x at 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.

command:
  - /bin/bash
  - -c
  - |
      set -x
      pip install ...
      mlflow server \
        --backend-store-uri "${MLFLOW_BACKEND_STORE_URI}" \
        ...

Then start the compose with

docker compose up -d

Then check the logs with

docker logs mlflow-server

You should see the real expanded commands prefixed with +, for example:

+ mlflow server --backend-store-uri postgresql://... --default-artifact-root s3://... --host 0.0.0.0 --port 5000

How is this PR tested?

  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests

Does this PR require documentation update?

  • No. You can skip the rest of this section.
  • Yes. I've updated:
    • Examples
    • API references
    • Instructions

Release Notes

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/tracking: Tracking Service, tracking client APIs, autologging
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflows
  • area/gateway: MLflow AI Gateway client APIs, server, and third-party integrations
  • area/prompts: MLflow prompt engineering features, prompt templates, and prompt management
  • area/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionality
  • area/projects: MLproject format, project running backends
  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages

How 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" section
  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

Should this PR be included in the next patch release?

Yes should be selected for bug fixes, documentation updates, and other small changes. No should 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?
  • Minor release: a release that increments the second part of the version number (e.g., 1.2.0 -> 1.3.0).
    Bug fixes, doc updates and new features usually go into minor releases.
  • Patch release: a release that increments the third part of the version number (e.g., 1.2.0 -> 1.2.1).
    Bug fixes and doc updates usually go into patch releases.
  • Yes (this PR will be cherry-picked and included in the next patch release)
  • No (this PR will be included in the next minor release)

@github-actions
Copy link
Contributor

@UnfixedMold Thank you for the contribution! Could you fix the following issue(s)?

⚠ DCO check

The 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 \
Copy link
Member

Choose a reason for hiding this comment

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

can we just remove \?

Copy link
Contributor Author

@UnfixedMold UnfixedMold Nov 19, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@UnfixedMold UnfixedMold Nov 19, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@B-Step62 B-Step62 Nov 19, 2025

Choose a reason for hiding this comment

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

Sorry I confused > and |, yea it seems the pipe doesn't concatenate the lines into a single command. Thanks for testing.

@github-actions github-actions bot added the rn/none List under Small Changes in Changelogs. label Nov 19, 2025
Copy link
Member

@harupy harupy left a comment

Choose a reason for hiding this comment

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

LGTM!

@harupy
Copy link
Member

harupy commented Nov 19, 2025

can you fix the lint error?

@UnfixedMold
Copy link
Contributor Author

Yeah sure, I'll address the lint errors

@harupy
Copy link
Member

harupy commented Nov 19, 2025

looks like unrelated commits are included in this PR. Can you remove them?

@UnfixedMold
Copy link
Contributor Author

UnfixedMold commented Nov 19, 2025

@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?

@harupy
Copy link
Member

harupy commented Nov 19, 2025

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>
@UnfixedMold
Copy link
Contributor Author

@harupy fixed. Cherry picked only relevant commits.

@harupy harupy enabled auto-merge November 19, 2025 15:50
@github-actions github-actions bot added the area/docs Documentation issues label Nov 19, 2025
@harupy harupy disabled auto-merge November 19, 2025 16:26
@harupy harupy merged commit 49eeae0 into mlflow:master Nov 19, 2025
43 of 45 checks passed
Tian-Sky-Lan pushed a commit to Tian-Sky-Lan/mlflow that referenced this pull request Nov 24, 2025
Signed-off-by: Henrikas Antanas <henrikasgir@gmail.com>
Signed-off-by: Tian Lan <sky.blue266000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation issues rn/none List under Small Changes in Changelogs. v3.6.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG/INSTALLATION ISSUE] docker-compose config silently drops mlflow server flags due to multiline bash command

3 participants