[core|serve] Migrate serve imports from ray._private to ray._common#61363
[core|serve] Migrate serve imports from ray._private to ray._common#61363edoakes merged 26 commits intoray-project:masterfrom
Conversation
db77238 to
330e601
Compare
|
@abrarsheikh please review the PR and let me know your feedback |
There was a problem hiding this comment.
Code Review
This pull request effectively continues the migration of Serve imports from ray._private to ray._common, which is a great step towards a cleaner architecture and reducing dependencies on internal APIs. The changes are well-structured, moving utilities to _common and updating Serve's codebase to use them, while maintaining backward compatibility with re-exports. The addition of new tests and a verification script is also a solid contribution to ensure the migration is correct and prevent future regressions.
I have a few minor suggestions in the new test files to improve test specificity and code style, but overall this is a high-quality contribution.
- Add ray._common.tls_utils with generate_self_signed_tls_certs (moved from _private; _private re-exports for backward compatibility). - Add ray._common.logging_constants with LOGRECORD_STANDARD_ATTRS; update _private.ray_logging.constants to import from _common. - Add ray._common.runtime_env_uri re-exporting parse_uri and Protocol from _private.runtime_env for use by Serve schema. - Add ray._common.worker_compat with set_blocking_get_inside_async_warned so Serve can mute the async warning without importing _private.worker. Serve now imports only from ray._common for these utilities: - serve/schema.py: LOGRECORD_STANDARD_ATTRS, parse_uri - serve/tests/test_https_proxy.py: generate_self_signed_tls_certs - serve/__init__.py: set_blocking_get_inside_async_warned Includes tests for tls_utils, logging_constants, and runtime_env_uri, plus a verification script for serve having no direct ray._private imports. Ref: ray-project#53478 Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
… error messages - test_logging_constants: assert exact count (23) instead of >= 20 - test_runtime_env_uri: use package_name, assert exact parse_uri output - test_tls_utils: single with + two temp files, no manual cleanup - schema.py, logging_config.py: format LOGRECORD_STANDARD_ATTRS as set() in error messages for readable output Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
…rify script Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
e680d30 to
a966037
Compare
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
python/ray/_common/worker_compat.py
Outdated
|
|
||
| from typing import Any, Optional, Tuple | ||
|
|
||
| from ray._private import worker |
There was a problem hiding this comment.
if common depends on _private, i dont think that is any better than directly accessing _private. defer to @edoakes for recommendations on what we want to do here.
There was a problem hiding this comment.
agree with it. I reverted the worker_compat migration in this PR and deferred this direction pending @edoakes guidance
Rollback worker_compat/runtime_env_uri moves pending maintainer direction, remove redundant test module docstrings, and switch TLS cert helper callsites to ray._common without _private re-export. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
@abrarsheikh just pushed the changes, please review again |
Re-export generate_self_signed_tls_certs from ray._private.tls_utils to preserve backward compatibility for existing import paths. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Revert assistant-applied commits from this chat session and restore branch contents to the pre-session baseline. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
This reverts commit 85cf7d9. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Create an empty signed commit to retrigger premerge checks. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Remove python/ray/_private/tls_utils.py and python/ray/_private/ray_logging/constants.py since all call sites now import directly from ray._common. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
@abrarsheikh can you please review the changes again? |
Wrap LOGRECORD_STANDARD_ATTRS with set() in serve/schema.py error
message to display {...} instead of frozenset({...}).
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
please fix the premerge |
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
fixed |
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
how ya doing? @abrarsheikh |
|
Needs to be signed off from ray-core team members before merging. Someone will get to it soon :) |
…ate-serve-from-private-to-common # Conflicts: # python/ray/serve/schema.py
|
@abrarsheikh it's been 5 days since you approved the PR. can you please help me mege this PR asap? |
|
premerge is failing. that's needs to be green before we can merge |
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
all checks passed, can we merge it? |
|
@edoakes for core review. |
|
hi @kouroshHakha can you please merge this PR? |
|
seems like that @edoakes is offline several days |
MengjinYan
left a comment
There was a problem hiding this comment.
Looks good overall! Added nit comments.
Also, looks like the changes in the PR doesn't quite match the PR description. Can we update the PR description?
|
|
||
|
|
||
| def test_logrecord_standard_attrs_contains_standard_names(): | ||
| expected = {"message", "levelname", "name", "pathname", "lineno", "exc_info"} |
There was a problem hiding this comment.
Minor: I'm wondering if we should test the whole set?
| from enum import Enum | ||
|
|
||
| assert issubclass(LogKey, Enum) | ||
| assert LogKey.JOB_ID.value == "job_id" |
There was a problem hiding this comment.
Similar as above, wondering if we should test the full set of keys?
Test the complete LOGRECORD_STANDARD_ATTRS frozenset and all LogKey enum members instead of just a subset, as requested by MengjinYan. Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
|
@MengjinYan thanks for your feedback, I implemented your comments and update the pr description again. can you please review again? |
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Signed-off-by: mkdev11 <MkDev11@users.noreply.github.com>
Description
This PR continues the migration from
ray._privatetoray._commonso that Ray Serve (and other libraries) do not depend on internal private APIs. It migratestls_utilsand logging constants out of_privateinto_common, and updates all consumers (Serve, dashboard, client server) to import from the new locations.Changes:
ray._common.tls_utilsgenerate_self_signed_tls_certs()from_private.tls_utilsinto_common.tls_utils(it only depended on_common.network_utils)._private.tls_utilsre-exports it for backward compatibility.serve/tests/test_https_proxy.pynow imports fromray._common.tls_utils._private/grpc_utils.py,_private/test_utils.py,dashboard/agent.py,util/client/server/proxier.py,util/client/server/server.pyupdated.ray._common.logging_constantsLOGRECORD_STANDARD_ATTRS(frozenset),LOGGER_FLATTEN_KEYS, andLogKeyenum — all moved from_private.ray_logging.constants._private.ray_logging.constantsis deleted;_private.ray_logging.logging_confignow imports from_common.serve/schema.pynow importsLOGRECORD_STANDARD_ATTRSfromray._common.logging_constants.ray._common.filters/ray._common.formattersray._common.logging_constantsinstead ofray._private.ray_logging.constants.Tests:
ray/_common/tests/test_logging_constants.py— tests the fullLOGRECORD_STANDARD_ATTRSfrozenset, allLogKeyenum members, andLOGGER_FLATTEN_KEYS.ray/_common/tests/test_tls_utils.py— testsgenerate_self_signed_tls_certs.Related issues
Fixes #53478 (partial: migrates Serve-side imports for
tls_utilsand logging constants; remaining items likeruntime_env_uriandworker_compatcan be done in follow-up PRs).Additional information
generate_self_signed_tls_certsfrom_private.tls_utilscontinues to work via re-export._private.ray_logging.constantsis removed since all consumers have been updated.ray._private.worker.*as attribute access (e.g.global_worker,_global_node) without a direct import; migrating those would require exposing them via_commonand is left for a separate PR._common/#53457 (signal/semaphore to _common), [core] Migrate wait_for_condition and async_wait_for_condition from _private to _common #53652 (wait_for_condition to _common).