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

Allow control of some serve configuration via env vars #47533

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

timkpaine
Copy link
Contributor

Why are these changes needed?

When a serve app is launched, serve will startup automatically. In certain places like k8s, it can be difficult to preconfigure serve (e.g. in the ray-cluster helm chart there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it starts up automatically you may need to shut it down, then restart it, which is inconvenient.

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(


#: HTTP Port
DEFAULT_HTTP_PORT = 8000
DEFAULT_HTTP_PORT = int(os.environ.get("RAY_SERVE_DEFAULT_HTTP_PORT", 8000))

#: Uvicorn timeout_keep_alive Config
DEFAULT_UVICORN_KEEP_ALIVE_TIMEOUT_S = 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are more here and below, happy to expose all of them if it makes sense to do so

Copy link
Contributor

Choose a reason for hiding this comment

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

we can leave the surface area minimal for now

@GeneDer
Copy link
Contributor

GeneDer commented Sep 6, 2024

Lgtm!

@GeneDer GeneDer requested a review from edoakes September 6, 2024 15:00
@GeneDer GeneDer added the go add ONLY when ready to merge, run all tests label Sep 6, 2024
@timkpaine timkpaine marked this pull request as ready for review September 10, 2024 04:42
Comment on lines +19 to +21
DEFAULT_HTTP_ADDRESS = os.environ.get(
"RAY_SERVE_DEFAULT_HTTP_ADDRESS", "http://127.0.0.1:8000"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check what this one is used for and why it's different from the two below?

Copy link
Contributor

Choose a reason for hiding this comment

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

@GeneDer can you follow up on this one? going to merge the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting this one is actually not used anywhere. Let me follow up to just remove it


#: HTTP Port
DEFAULT_HTTP_PORT = 8000
DEFAULT_HTTP_PORT = int(os.environ.get("RAY_SERVE_DEFAULT_HTTP_PORT", 8000))

#: Uvicorn timeout_keep_alive Config
DEFAULT_UVICORN_KEEP_ALIVE_TIMEOUT_S = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

we can leave the surface area minimal for now

@edoakes edoakes merged commit ac7face into ray-project:master Sep 10, 2024
6 checks passed
@timkpaine timkpaine deleted the tkp/serveenvvar branch September 10, 2024 18:42
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…7533)

When a serve app is launched, serve will startup automatically. In
certain places like k8s, it can be difficult to preconfigure serve (e.g.
in the [ray-cluster helm
chart](https://github.com/ray-project/kuberay/blob/master/helm-chart/ray-cluster/values.yaml)
there is no ability to set the default serve arguments).

This means you need to either be explicit when you start serve, or if it
starts up automatically you may need to shut it down, then restart it,
which is inconvenient.

Signed-off-by: Tim Paine <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants