-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Reduce FFI boilerplate for ExecutionRequest
, PySession
, and PyTasks
#11700
Reduce FFI boilerplate for ExecutionRequest
, PySession
, and PyTasks
#11700
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
lgtm
def write_log(self, msg: str, *, level: int, target: str): | ||
"""Proxy a log message to the Rust logging faculties.""" | ||
return self.lib.write_log(msg, level, target) | ||
|
||
def flush_log(self): | ||
return self.lib.flush_log() |
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.
Unused
def new_tasks(self) -> PyTasks: | ||
return PyTasks() | ||
|
||
def new_execution_request(self) -> PyExecutionRequest: | ||
return PyExecutionRequest() | ||
|
||
def new_session( |
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.
Inlined
@@ -187,51 +190,6 @@ def _to_params_list(self, subject_or_params): | |||
return subject_or_params.params | |||
return [subject_or_params] | |||
|
|||
def _register_rules(self, rule_index: RuleIndex, union_membership: UnionMembership): |
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.
Moved to a free function at the bottom of the file.
def execution_set_timeout(self, execution_request, timeout: float): | ||
timeout_in_ms = int(timeout * 1000) | ||
self._native.lib.execution_set_timeout(execution_request, timeout_in_ms) | ||
|
||
def execution_set_poll(self, execution_request, poll: bool): | ||
self._native.lib.execution_set_poll(execution_request, poll) | ||
|
||
def execution_set_poll_delay(self, execution_request, poll_delay: float): | ||
poll_delay_in_ms = int(poll_delay * 1000) | ||
self._native.lib.execution_set_poll_delay(execution_request, poll_delay_in_ms) |
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.
Now directly specified in the PyExecutionRequest
constructor.
@@ -679,3 +624,48 @@ def get_observation_histograms(self): | |||
|
|||
def record_test_observation(self, value: int) -> None: | |||
self._scheduler.record_test_observation(self._session, value) | |||
|
|||
|
|||
def register_rules(rule_index: RuleIndex, union_membership: UnionMembership) -> PyTasks: |
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.
Lmk if there's a better place to put this. Maybe native.py
? We won't need the Native
singleton because CPython allows us to directly access native_engine.so
, and we are slowly getting rid of Externs
, but we could still use that file to collect helper functions like this?
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.
This should be fine... exploding access to native_engine
out across multiple modules makes sense, because if we could (easily) split native_engine
we would.
ExecuteRequest
, PySession
, and PyTasks
ExecutionRequest
, PySession
, and PyTasks
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This brings us closer to idiomatic CPython usage and also adds Python type hints to these types.