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.

Sign up for GitHub

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

Jump to bottom

fix: honour server port when coming from config #1105

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: honour server port when coming from config #1105

wants to merge 1 commit into from
+125 −5

Conversation

leseb
Copy link
Contributor

@leseb leseb commented Feb 14, 2025 •
edited
Loading

What does this PR do?

Ensure that the options follow the correct priority order:

  1. Environment variable (eg: LLAMA_STACK_PORT) takes the highest precedence.
  2. CLI flag (--port) is considered only if the env variable is unset.
  3. Config file value (config.server.port) is used as a fallback.
  4. Default port (8321) is the last resort.

Closes: #1076

Test Plan

  1. Run a server with LLAMA_STACK_PORT=7000, notice the server listening on that port
  2. Run a server with --port 8000, notice the server listening on that port
  3. Set 9000 to server.port in your run config file, notice the server listening on that port
  4. Do nothing, notice the server listening on the default port 8123
uv run pytest -v tests/unit/test_server.py
/Users/leseb/Documents/AI/llama-stack/.venv/lib/python3.12/site-packages/pytest_asyncio/plugin.py:207: PytestDeprecationWarning: The configuration option "asyncio_default_fixture_loop_scope" is unset.
The event loop scope for asynchronous fixtures will default to the fixture caching scope. Future versions of pytest-asyncio will default the loop scope for asynchronous fixtures to function scope. Set the default fixture loop scope explicitly in order to avoid unexpected behavior in the future. Valid fixture loop scopes are: "function", "class", "module", "package", "session"

  warnings.warn(PytestDeprecationWarning(_DEFAULT_FIXTURE_LOOP_SCOPE_UNSET))
========================================== test session starts ==========================================
platform darwin -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0 -- /Users/leseb/Documents/AI/llama-stack/.venv/bin/python3
cachedir: .pytest_cache
rootdir: /Users/leseb/Documents/AI/llama-stack
configfile: pyproject.toml
plugins: asyncio-0.25.3, anyio-4.8.0, nbval-0.11.0
asyncio: mode=Mode.STRICT, asyncio_default_fixture_loop_scope=None
collected 4 items                                                                                       

llama_stack/distribution/tests/test_server.py::test_return_flag_value[flag1-ENV_FLAG1-env_value1-args0-config_value1-default_args0-env_value1] PASSED [ 25%]
llama_stack/distribution/tests/test_server.py::test_return_flag_value[flag2-ENV_FLAG2-None-args1-config_value2-default_args1-cli_value2] PASSED [ 50%]
llama_stack/distribution/tests/test_server.py::test_return_flag_value[flag3-ENV_FLAG3-None-args2-config_value3-default_args2-config_value3] PASSED [ 75%]
llama_stack/distribution/tests/test_server.py::test_return_flag_value[flag4-ENV_FLAG4-None-args3-None-default_args3-default_value4] PASSED [100%]

=========================================== warnings summary ============================================
.venv/lib/python3.12/site-packages/pydantic/fields.py:1042
  /Users/leseb/Documents/AI/llama-stack/.venv/lib/python3.12/site-packages/pydantic/fields.py:1042: PydanticDeprecatedSince20: Using extra keyword arguments on `Field` is deprecated and will be removed. Use `json_schema_extra` instead. (Extra keys: 'contentEncoding'). Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.10/migration/
    warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===================================== 4 passed, 1 warning in 0.60s ======================================

Unit tests need #1004 to run properly.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Feb 14, 2025
@leseb
Copy link
Contributor Author

leseb commented Feb 14, 2025

I would like to add unit test for this, but given the current structure of the test, I'm seeking some guidance on where the right place should be. Thanks!

booxter
booxter suggested changes Feb 18, 2025
View reviewed changes
@leseb leseb requested a review from booxter February 18, 2025 21:54
booxter
booxter approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@booxter booxter left a comment

Choose a reason for hiding this comment

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

Thank you.

@leseb leseb force-pushed the fix-1076 branch 2 times, most recently from 8d4b81f to bb8cc99 Compare February 19, 2025 09:34
@leseb leseb requested a review from booxter February 19, 2025 09:34
ashwinb
ashwinb requested changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

Honestly, I don't know if we need to support this much complexity here at all. What if we just allowed two ways to specify a port?

  1. config.server.port is obviously the most reasonable path and is choice number one
  2. followed by --port which takes precedence if specified

We should kill the environment variable use of LLAMA_STACK_PORT

@leseb
Copy link
Contributor Author

leseb commented Feb 20, 2025

Honestly, I don't know if we need to support this much complexity here at all. What if we just allowed two ways to specify a port?

  1. config.server.port is obviously the most reasonable path and is choice number one
  2. followed by --port which takes precedence if specified

We should kill the environment variable use of LLAMA_STACK_PORT

The environment variable is quite common in my opinion. CLI tools like Click, for example, support it natively. I don’t have a strong preference, but environment variables can be very useful.

@ashwinb I don't mind removing the env support though, what's the final decision? Thanks!

@leseb leseb requested a review from ashwinb February 24, 2025 11:21
@leseb
fix: honour server port when coming from config
2daca32 
Ensure that the options follow the correct priority order:

1. Environment variable (eg: `LLAMA_STACK_PORT`) takes the highest precedence.
2. CLI flag (`--port`) is considered only if the env variable is unset.
3. Config file value (`config.server.port`) is used as a fallback.
4. Default port (`8321`) is the last resort.

Closes: meta-llama#1076
Signed-off-by: Sébastien Han <[email protected]>
@leseb leseb requested a review from hardikjshah February 24, 2025 13:52
@thoraxe
Copy link

thoraxe commented Feb 24, 2025

FWIW environment variables are extremely common in Kubernetes environments for configuring the way things run. I would agree that supporting ENV is valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@booxter booxter booxter approved these changes

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 is a code owner

@dltn dltn Awaiting requested review from dltn dltn is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

@dineshyv dineshyv Awaiting requested review from dineshyv dineshyv is a code owner

@vladimirivic vladimirivic Awaiting requested review from vladimirivic vladimirivic is a code owner

@sixianyi0721 sixianyi0721 Awaiting requested review from sixianyi0721 sixianyi0721 is a code owner

@ehhuang ehhuang Awaiting requested review from ehhuang ehhuang is a code owner

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan is a code owner

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

server defaults to port 8321 ignoring configured server port from yaml
6 participants
@leseb @thoraxe @ashwinb @booxter @hardikjshah @facebook-github-bot