-
Notifications
You must be signed in to change notification settings - Fork 5
fix(ray): propagate original function name to Ray metrics instead of generic _capture [ENG-358] #128
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(ray): propagate original function name to Ray metrics instead of generic _capture [ENG-358] #128
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,7 +180,7 @@ def execute_callable( | |
| import ray | ||
|
|
||
| self._ensure_ray_initialized() | ||
| remote_fn = self._get_remote_fn(ray) | ||
| remote_fn = self._get_remote_fn(ray, fn.__name__) | ||
| ref = remote_fn.options(name=fn.__name__).remote(fn, kwargs) | ||
|
|
||
| try: | ||
|
|
@@ -206,7 +206,7 @@ async def async_execute_callable( | |
| import ray | ||
|
|
||
| self._ensure_ray_initialized() | ||
| remote_fn = self._get_remote_fn(ray) | ||
| remote_fn = self._get_remote_fn(ray, fn.__name__) | ||
| ref = remote_fn.options(name=fn.__name__).remote(fn, kwargs) | ||
|
Comment on lines
182
to
210
|
||
|
|
||
| try: | ||
|
|
@@ -225,26 +225,26 @@ async def async_execute_callable( | |
| self._record_success(stdout_log, stderr_log, python_logs, logger) | ||
| return raw | ||
|
|
||
| def _get_remote_fn(self, ray: Any) -> Any: | ||
| def _get_remote_fn(self, ray: Any, fn_name: str) -> Any: | ||
| """Return a cached Ray remote wrapper for the capture closure. | ||
|
|
||
| The capture wrapper's bytecode is identical on every invocation, so | ||
| it only needs to be remotized once per distinct set of remote options. | ||
| Caching avoids the non-trivial overhead of ``ray.remote()`` on every | ||
| packet. | ||
| The capture wrapper is created with ``fn_name`` so that Ray's | ||
| metrics and dashboard report the original function name instead | ||
| of the generic ``_capture``. Wrappers are cached per distinct | ||
| ``(remote_opts, fn_name)`` pair. | ||
|
|
||
| A ``threading.Lock`` guards population so that concurrent calls | ||
| (``supports_concurrent_execution = True``) never redundantly call | ||
| ``ray.remote()`` for the same option set. | ||
| """ | ||
| opts = self._build_remote_opts() | ||
| cache_key = self._normalize_opts(opts) | ||
| cache_key = (self._normalize_opts(opts), fn_name) | ||
| if cache_key not in self._remote_fn_cache: | ||
| with self._remote_fn_cache_lock: | ||
| # Double-checked: another thread may have filled the slot | ||
| # while we waited for the lock. | ||
| if cache_key not in self._remote_fn_cache: | ||
| wrapper = make_capture_wrapper() | ||
| wrapper = make_capture_wrapper(name=fn_name) | ||
| self._remote_fn_cache[cache_key] = ray.remote(**opts)(wrapper) | ||
|
Comment on lines
240
to
248
|
||
| return self._remote_fn_cache[cache_key] | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior (setting wrapper name/qualname when
nameis provided) isn't covered by tests. Please add a unit test asserting thatmake_capture_wrapper(name="...")returns a callable whose__name__and__qualname__match the provided name (and that defaultNonekeeps the original_capturename).