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 log format for serve #28760

Merged
merged 4 commits into from
Nov 9, 2022
Merged

Conversation

vitrioil
Copy link
Contributor

Why are these changes needed?

There is an error while logging Connecting to existing Serve app... leading to an exception being printed in the logs.

--- Logging error ---
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/logging/__init__.py", line 1079, in emit
    msg = self.format(record)
  File "/usr/local/lib/python3.9/logging/__init__.py", line 923, in format
    return fmt.format(record)
  File "/usr/local/lib/python3.9/logging/__init__.py", line 659, in format
    record.message = record.getMessage()
  File "/usr/local/lib/python3.9/logging/__init__.py", line 363, in getMessage
    msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
  File "/home/user/app/main.py", line 12, in <module>
    serve.start(detached=True)
  File "/home/user/.cache/pypoetry/virtualenvs/delta-Ost0U_JB-py3.9/lib/python3.9/site-packages/ray/serve/api.py", line 99, in start
    client = _private_api.serve_start(detached, http_options, dedicated_cpu, **kwargs)
  File "/home/user/.cache/pypoetry/virtualenvs/delta-Ost0U_JB-py3.9/lib/python3.9/site-packages/ray/serve/_private/api.py", line 173, in serve_start
    logger.info(
Message: 'Connecting to existing Serve app in namespace "serve".'
Arguments: (' New http options will not be applied.',)

Sample code to generate the error:

from ray import serve
import logging
logging.basicConfig(level=logging.INFO)
serve.start(detached=True)

After the changes it logs:
Connecting to existing Serve app in namespace "serve". New http options will not be applied.

Related issue number

Closes #28567

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 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 :(

@architkulkarni
Copy link
Contributor

@vitrioil Thanks for the contribution! Do you mind adding a quick test for this, perhaps in serve/tests/test_logging.py or serve/tests/test_logs.py? You can use the capsys pytest fixture to verify that the entire string appears in the output.

@architkulkarni architkulkarni added the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Oct 28, 2022
@simon-mo
Copy link
Contributor

simon-mo commented Nov 4, 2022

Hi @vitrioil would you be able to update this PR? Alternatively, @architkulkarni can you help if you have free cycle?

@vitrioil
Copy link
Contributor Author

vitrioil commented Nov 5, 2022

Apologies!

I was working on this, but I think for running tests locally I have to do a full bazel build, if I'm not completely wrong?

When I tried to do that it went out of memory and I couldn't run tests locally.

@architkulkarni
Copy link
Contributor

architkulkarni commented Nov 7, 2022

Hi @vitrioil, good question, and sorry for the poor experience here -- we want to make things as smooth as possible for new contributors so your feedback is appreciated.

Can you try one of these?

If your machine is running out of memory during the build or the build is causing other programs to crash, try adding the following line to ~/.bazelrc:

build --local_ram_resources=HOST_RAM*.5 --local_cpu_resources=4

The build --disk_cache=~/bazel-cache option can be useful to speed up repeated builds too.

@vitrioil
Copy link
Contributor Author

vitrioil commented Nov 8, 2022

@architkulkarni thanks for the suggestion, I can run bazel command with that.

bazel build --verbose_failures -- //:ray_pkg 

However the final pip install still takes a lot of memory and crashes.

 pip install -e .

I am doing this because running test requires _ray import.

I guess the latter one is needed to locally install?

Anyway, I've to push the test and see if it works in build.


# wait long enough for the warning to be printed
# with a small grace period
time.sleep(SLOW_STARTUP_WARNING_PERIOD_S * 1.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we would use wait_for_condition instead of a fixed sleep here, but no need to block the PR on this

@architkulkarni
Copy link
Contributor

Ah interesting, sorry about that -- I'm not sure why the bazel build succeeds while the pip install doesn't.

Any success with the approach in https://docs.ray.io/en/latest/ray-contribute/development.html#building-ray-python-only with python python/ray/setup-dev.py?

(On an unrelated note, it looks like the CI didn't run on the latest commit for some reason, can you try merging master and pushing again, or pushing a new commit?)

@architkulkarni architkulkarni merged commit bb7bfc3 into ray-project:master Nov 9, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
There is an error while logging Connecting to existing Serve app... leading to an exception being printed in the logs.

--- Logging error ---
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/logging/__init__.py", line 1079, in emit
    msg = self.format(record)
  File "/usr/local/lib/python3.9/logging/__init__.py", line 923, in format
    return fmt.format(record)
  File "/usr/local/lib/python3.9/logging/__init__.py", line 659, in format
    record.message = record.getMessage()
  File "/usr/local/lib/python3.9/logging/__init__.py", line 363, in getMessage
    msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
  File "/home/user/app/main.py", line 12, in <module>
    serve.start(detached=True)
  File "/home/user/.cache/pypoetry/virtualenvs/delta-Ost0U_JB-py3.9/lib/python3.9/site-packages/ray/serve/api.py", line 99, in start
    client = _private_api.serve_start(detached, http_options, dedicated_cpu, **kwargs)
  File "/home/user/.cache/pypoetry/virtualenvs/delta-Ost0U_JB-py3.9/lib/python3.9/site-packages/ray/serve/_private/api.py", line 173, in serve_start
    logger.info(
Message: 'Connecting to existing Serve app in namespace "serve".'
Arguments: (' New http options will not be applied.',)
Sample code to generate the error:

from ray import serve
import logging
logging.basicConfig(level=logging.INFO)
serve.start(detached=True)
After the changes it logs:
Connecting to existing Serve app in namespace "serve". New http options will not be applied.

Signed-off-by: Weichen Xu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Serve] Logger syntax is wrong
4 participants