-
Notifications
You must be signed in to change notification settings - Fork 59
Implement interface for calling PEP 517 hooks #732
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
Changes from 2 commits
6798cd6
06fce2a
fb31156
70c8019
b263c42
0727c07
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 |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| # Copyright 2022 Open Source Robotics Foundation, Inc. | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| from contextlib import AbstractContextManager | ||
| import os | ||
| import pickle | ||
| import sys | ||
|
|
||
| from colcon_core.python_project.hook_caller import _call_hook | ||
| from colcon_core.python_project.hook_caller import _list_hooks | ||
| from colcon_core.python_project.spec import load_and_cache_spec | ||
| from colcon_core.subprocess import run | ||
|
|
||
|
|
||
| class _SubprocessTransport(AbstractContextManager): | ||
|
|
||
| def __enter__(self): | ||
| self.child_in, self.parent_out = os.pipe() | ||
| self.parent_in, self.child_out = os.pipe() | ||
|
|
||
| try: | ||
| import msvcrt | ||
| except ImportError: | ||
| os.set_inheritable(self.child_in, True) | ||
| self.pass_in = self.child_in | ||
| os.set_inheritable(self.child_out, True) | ||
| self.pass_out = self.child_out | ||
| else: | ||
| self.pass_in = msvcrt.get_osfhandle(self.child_in) | ||
| os.set_handle_inheritable(self.pass_in, True) | ||
| self.pass_out = msvcrt.get_osfhandle(self.child_out) | ||
| os.set_handle_inheritable(self.pass_out, True) | ||
|
|
||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, traceback): | ||
| os.close(self.parent_out) | ||
| os.close(self.parent_in) | ||
| os.close(self.child_out) | ||
| os.close(self.child_in) | ||
|
|
||
|
|
||
| class AsyncHookCaller: | ||
| """Calls PEP 517 style hooks asynchronously in a new process.""" | ||
|
|
||
| def __init__( | ||
| self, backend_name, *, project_path=None, env=None, | ||
| stdout_callback=None, stderr_callback=None, | ||
| ): | ||
| """ | ||
| Initialize a new AsyncHookCaller. | ||
|
|
||
| :param backend_name: The name of the PEP 517 build backend. | ||
| :param project_path: Path to the project's root directory. | ||
| :param env: Environment variables to use when invoking hooks. | ||
| :param stdout_callback: Callback for stdout from the hook invocation. | ||
| :param stderr_callback: Callback for stderr from the hook invocation. | ||
| """ | ||
| self._backend_name = backend_name | ||
| self._project_path = str(project_path) if project_path else None | ||
| self._env = dict(env if env is not None else os.environ) | ||
| self._stdout_callback = stdout_callback | ||
| self._stderr_callback = stderr_callback | ||
|
|
||
| @property | ||
| def backend_name(self): | ||
| """Get the name of the backend to call hooks on.""" | ||
| return self._backend_name | ||
|
|
||
| @property | ||
| def env(self): | ||
| """Get the environment variables to use when invoking hooks.""" | ||
| return self._env | ||
|
|
||
| async def list_hooks(self): | ||
| """ | ||
| Call into the backend to list implemented hooks. | ||
|
|
||
| This function lists all callable methods on the backend, which may | ||
| include more than just the hook names. | ||
|
|
||
| :returns: List of hook names. | ||
| """ | ||
| args = [ | ||
| sys.executable, _list_hooks.__file__, | ||
| self._backend_name] | ||
| process = await run( | ||
| args, None, self._stderr_callback, | ||
| cwd=self._project_path, env=self.env, | ||
| capture_output=True) | ||
| process.check_returncode() | ||
| hook_names = [ | ||
| line.strip().decode() for line in process.stdout.splitlines()] | ||
| return [ | ||
| hook for hook in hook_names if hook and not hook.startswith('_')] | ||
|
|
||
| async def call_hook(self, hook_name, **kwargs): | ||
| """ | ||
| Call the given hook with given arguments. | ||
|
|
||
| :param hook_name: Name of the hook to call. | ||
| """ | ||
| with _SubprocessTransport() as transport: | ||
| args = [ | ||
| sys.executable, _call_hook.__file__, | ||
| self._backend_name, hook_name, | ||
| str(transport.pass_in), str(transport.pass_out)] | ||
| with os.fdopen(os.dup(transport.parent_out), 'wb') as f: | ||
| pickle.dump(kwargs, f) | ||
| have_callbacks = self._stdout_callback or self._stderr_callback | ||
| process = await run( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question about the performance of the process-per-hook design, specifically in the new
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is absolutely a problem, and one of my biggest critiques of PEP 517. In the worst case, we may actually need to build a project just to determine what the name of the package is. :( Still, a worst-case scenario will only make three subprocess invocations during a typical colcon operation:
In reality, we'll rarely do (1). For ROS packages, we'll have tl;dr - process invocations are indeed slow, but I think we can mitigate the fallout. In the end, it's more important to conform to PEP 517:
|
||
| args, self._stdout_callback, self._stderr_callback, | ||
| cwd=self._project_path, env=self.env, close_fds=False, | ||
| capture_output=not have_callbacks) | ||
| process.check_returncode() | ||
| with os.fdopen(os.dup(transport.parent_in), 'rb') as f: | ||
| res = pickle.load(f) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a potential deadlock issue here related to IPC handling within the event loop. Because While this might not affect basic use cases with small payloads, it is possible to freeze in complex bigger scenarios (large dependency list or extensive config_settings). Has a fully non-blocking IPC approach using asyncio streams been considered to make this more robust for advanced use cases? Maybe this is over-optimizing and can be considered if an issue is found and asyncio would remove the low level OS pipes from the previous comment.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, that's a good point. I see some options to consider:
Thoughts?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After moving from Pickle to JSON, the read/write operations got separated from the (de)serialization anyway, so I went ahead and did (1) in 70c8019.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof, actually it looks like that won't work on Windows. Reverted.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just pushed the write to a background thread in 0727c07, which should improve this scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About the use of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the hooks defined today, they all seem to accept and return only Python primitives. I'm willing to bet that Pickle will be more performant, but we can't limit Pickle to only Python primitives, so switching to another serializer will give us that security (indirectly). I'll give JSON a shot.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to use JSON in fb31156. It was a pretty clean change. |
||
| return res | ||
|
|
||
|
|
||
| def get_hook_caller(desc, **kwargs): | ||
| """ | ||
| Create a new AsyncHookCaller instance for a package descriptor. | ||
|
|
||
| :param desc: The package descriptor | ||
| """ | ||
| spec = load_and_cache_spec(desc) | ||
| backend_path = spec['build-system'].get('backend-path') | ||
| if backend_path: | ||
| # TODO: This isn't *technically* the beginning of sys.path | ||
| # as PEP 517 calls for, but it's pretty darn close. | ||
| kwargs['env'] = { | ||
| **kwargs.get('env', os.environ), | ||
| 'PYTHONDONTWRITEBYTECODE': '1', | ||
| } | ||
| pythonpath = kwargs['env'].get('PYTHONPATH', '') | ||
| kwargs['env']['PYTHONPATH'] = os.pathsep.join( | ||
| backend_path + ([pythonpath] if pythonpath else [])) | ||
| return AsyncHookCaller( | ||
| spec['build-system']['build-backend'], | ||
| project_path=desc.path, **kwargs) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Copyright 2022 Open Source Robotics Foundation, Inc. | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| from importlib import import_module | ||
| import os | ||
| import pickle | ||
| import sys | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| backend_name, hook_name, child_in, child_out = sys.argv[1:] | ||
| try: | ||
| import msvcrt | ||
| except ImportError: | ||
| pass | ||
| else: | ||
| child_in = msvcrt.open_osfhandle(int(child_in), os.O_RDONLY) | ||
| child_out = msvcrt.open_osfhandle(int(child_out), 0) | ||
| if ':' in backend_name: | ||
| backend_module_name, backend_object_name = backend_name.split(':', 2) | ||
| backend_module = import_module(backend_module_name) | ||
| backend = getattr(backend_module, backend_object_name) | ||
| else: | ||
| backend = import_module(backend_name) | ||
| with os.fdopen(int(child_in), 'rb') as f: | ||
| kwargs = pickle.load(f) or {} | ||
| res = getattr(backend, hook_name)(**kwargs) | ||
| with os.fdopen(int(child_out), 'wb') as f: | ||
| pickle.dump(res, f) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # Copyright 2023 Open Source Robotics Foundation, Inc. | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| from importlib import import_module | ||
| import sys | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| backend_name = sys.argv[1] | ||
| if ':' in backend_name: | ||
| backend_module_name, backend_object_name = backend_name.split(':', 2) | ||
| backend_module = import_module(backend_module_name) | ||
| backend = getattr(backend_module, backend_object_name) | ||
| else: | ||
| backend = import_module(backend_name) | ||
|
|
||
| for attr in dir(backend): | ||
| if callable(getattr(backend, attr)): | ||
| print(attr) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # Copyright 2022 Open Source Robotics Foundation, Inc. | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| import traceback | ||
|
|
||
| from colcon_core.logging import colcon_logger | ||
| from colcon_core.plugin_system import instantiate_extensions | ||
| from colcon_core.plugin_system import order_extensions_by_priority | ||
| from colcon_core.python_project.hook_caller import get_hook_caller | ||
|
|
||
| logger = colcon_logger.getChild(__name__) | ||
|
|
||
|
|
||
| class HookCallerDecoratorExtensionPoint: | ||
| """ | ||
| The interface for PEP 517 hook caller decorator extensions. | ||
|
|
||
| For each instance the attribute `HOOK_CALLER_DECORATOR_NAME` is being | ||
| set to the basename of the entry point registering the extension. | ||
| """ | ||
|
|
||
| """The version of the hook caller decorator extension interface.""" | ||
| EXTENSION_POINT_VERSION = '1.0' | ||
|
|
||
| """The default priority of hook caller decorator extensions.""" | ||
| PRIORITY = 100 | ||
|
|
||
| def decorate_hook_caller(self, *, hook_caller): | ||
| """ | ||
| Decorate a hook caller to perform additional functionality. | ||
|
|
||
| This method must be overridden in a subclass. | ||
|
|
||
| :param hook_caller: The hook caller | ||
| :returns: A decorator | ||
| """ | ||
| raise NotImplementedError() | ||
|
|
||
|
|
||
| def get_hook_caller_extensions(): | ||
| """ | ||
| Get the available hook caller decorator extensions. | ||
|
|
||
| The extensions are ordered by their priority and entry point name. | ||
|
|
||
| :rtype: OrderedDict | ||
| """ | ||
| extensions = instantiate_extensions(__name__) | ||
| for name, extension in extensions.items(): | ||
| extension.HOOK_CALLER_DECORATOR_NAME = name | ||
| return order_extensions_by_priority(extensions) | ||
|
|
||
|
|
||
| def decorate_hook_caller(hook_caller): | ||
| """ | ||
| Decorate the hook caller using hook caller decorator extensions. | ||
|
|
||
| :param hook_caller: The hook caller | ||
|
|
||
| :returns: The decorated parser | ||
| """ | ||
| extensions = get_hook_caller_extensions() | ||
| for extension in extensions.values(): | ||
| logger.log( | ||
| 1, 'decorate_hook_caller() %s', | ||
| extension.HOOK_CALLER_DECORATOR_NAME) | ||
| try: | ||
| decorated_hook_caller = extension.decorate_hook_caller( | ||
| hook_caller=hook_caller) | ||
| assert hasattr(decorated_hook_caller, 'call_hook'), \ | ||
| 'decorate_hook_caller() should return something to call hooks' | ||
| except Exception as e: # noqa: F841 | ||
| # catch exceptions raised in decorator extension | ||
| exc = traceback.format_exc() | ||
| logger.error( | ||
| 'Exception in hook caller decorator extension ' | ||
| f"'{extension.HOOK_CALLER_DECORATOR_NAME}': {e}\n{exc}") | ||
| # skip failing extension, continue with next one | ||
| else: | ||
| hook_caller = decorated_hook_caller | ||
|
|
||
| return hook_caller | ||
|
|
||
|
|
||
| def get_decorated_hook_caller(desc, **kwargs): | ||
| """ | ||
| Create and decorate a hook caller instance for a package descriptor. | ||
|
|
||
| :param desc: The package descriptor | ||
| """ | ||
| hook_caller = get_hook_caller(desc, **kwargs) | ||
| decorated_hook_caller = decorate_hook_caller(hook_caller) | ||
| return decorated_hook_caller |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,9 @@ def test_flake8(): | |
| show_source=True, | ||
| ) | ||
| style_guide_tests = get_style_guide( | ||
| extend_ignore=['D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D107'], | ||
| extend_ignore=[ | ||
| 'D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107', | ||
| ], | ||
|
Comment on lines
+28
to
+30
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||
| show_source=True, | ||
| ) | ||
|
|
||
|
|
||
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.
I'm curious about the
_SubprocessTransportclass here. Manually handling low-level OS pipes and platform-specific inheritance logic (likemsvcrt) can be brittle. Incolcon_core/subprocess.pythe subprocess management is done usingasyncio.create_subprocess_execif I'm not wrong. Wouldn't be posible to simplify the IPC by using asyncio? Leveragingasyncio.create_subprocess_execwithstdin=asyncio.subprocess.PIPEandstdout=asyncio.subprocess.PIPEdirectly could align better with existing implementation and address the possible deadlock concern in one of the next comments.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 is a great question. The problem with using stdin/stdout is that PEP 517 doesn't forbid backends from using stdout for normal console messages, so we can't use it as a data pipeline. The purpose of this class is really just to set up additional pipes which behave very similar to stdin/stdout already do, but with a dedicated purpose.
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.
Oh, I see. Thank you for the clarification.