-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Pants plugins: pack_metadata - python imports rules #6260
Pants plugins: pack_metadata - python imports rules #6260
Conversation
db829b3
to
fc65ba4
Compare
@@ -28,7 +28,9 @@ | |||
|
|||
def rules(): | |||
return [ | |||
PythonTestsGeneratorTarget.register_plugin_field(UsesServicesField), | |||
PythonTestsGeneratorTarget.register_plugin_field( | |||
UsesServicesField, as_moved_field=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.
Minor fix. This is unrelated to the rest of this PR, other than, I noticed as_moved_field
was missing here when I added InjectPythonPathField
. Luckily, it still worked, but this is more correct.
I also corrected the dependencies field on pack_metadata
to be a moved field. Being a "moved" field means you can define it on a target generator, but the actual metadata for the field ends up on the generated target, not on the target generator (so, on pack_content_resource
instead of on pack_metadata
).
Now pack_metadata targets will generate pack_content_resource instead of just resource. pack_content_resource is still a resource, but this setup allows us to find the generated resource targets more simply. This also harmonizes the implementation of pack_metadata to follow the fields definition of resources (esp moving dependencies into moved_fields instead of core_fields).
…esource targets This will allow rules to look up just action and sensor metadata (for example).
… module mapping Only handles the actual action/sensor python files. It does not yet handle: - <pack>/lib - <pack>/actions/lib
…thon module mapping This makes dependency inference aware of these which may be on the PYTHONPATH. - <pack>/lib - <pack>/actions/lib
The pack_metadata plugin now handles identifying these imports for dep inference. Next step, modify the PYTHONPATH as well.
…_content and related rules
…TRA_SYS_PATH for tests This won't work until pants gets support for injecting path entries.
…xture Writes various test scenarios in the test sandbox. This should avoid repeating the setup in every test.
And fix the identified issues.
…ules And fix the identified mistake in conftest.
…_dependencies rule
fc65ba4
to
b3d8c4c
Compare
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, nice job!
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 will trust you on this one. great explanation. I admit this is a bit beyond my expertise.
This PR has commits extracted from #6202 where I'm working on getting all of our unit tests to run under pants+pytest.
It is best to review each commit of this PR to make the review manageable (I rewrote history to simplify reviews). If this PR is still too large to review, I can try to split it into multiple PRs.
Overview
This has pants+pytest run pack tests directly instead of using
st2-run-pack-tests
, which has some issues detailed below.This enhances
pants-plugins/pack_metadata
(introduced in #5868) so that it can inject the customPYTHONPATH
entries that StackStorm adds when running python in packs like actions and sensors. We have several special lib directories to support:<pack>/lib
is the shared lib dir that gets added for both actions and sensors when[packs].enable_common_libs = True
inst2.conf
.<pack>/actions/lib
for actions (allowingimport foo
to import<pack>/actions/lib/foo.py
in an action)entry_point
parent directory for sensors and actionsThat last point is actually not handled correctly in
st2-run-pack-tests
which only adds<pack>/actions/
and<pack>/sensors/
to PYTHONPATH, which assumes that the python file is directly in those directories. The python files can, however be in a sub directory, and that sub directory gets added to PYTHONPATH. So, thispants-plugins/pack_metadata
enhancement allows pants to accurately reflect the logic st2 actually uses.Another reason that this does not use
st2-run-pack-tests
, is that it obscures thePYTHONPATH
logic that pants needs to carefully manage. Pants uses PEX to make sure that tests are run in a hermetic sandbox that cannot be influenced by things like packages installed withpip install --user
. So, the python dependencies detected by this plugin actually get passed asPEX_EXTRA_SYS_PATH
notPYTHONPATH
which is ignored when running python hermetically.Eventually, I would like to publish
pants-plugins/pack_metadata
so it can be used in pack repos to run tests with pants+pytest instead ofst2-run-pack-tests
.Plugin components
Developing pants plugins
Pants has extensive documentation on the plugin API, including how to create targets, and how to write rules. Some of the plugin APIs I'm using in this PR are not documented, but these still give the general overview of the rule+target mechanisms used.
Targets and Target Fields
pack_metadata
andpack_content_resource
Targets and their FieldsThe
pack_metadata
target was introduced in #5868, and it was added to BUILD files in #5871. For example, this is thepack_metadata
target for thelinux
pack:st2/contrib/linux/BUILD
Lines 3 to 5 in b3d8c4c
Creating this doesn't require manual intervention. If a new pack is added, CI will prompt you to run
pants tailor ::
which generates this target (see #5871). One of my goals with this PR is to preserve this low-maintenance aspect while introducing the features we need.So, what does the
pack_metadata
target do? It is basically aresources
target that generates aresource
target for each of the yaml files in the pack: pack.yaml, action metadata, sensor metadata, rules, etc. This is similar to thepython_sources
target which generates apython_source
target for each python file.In order to calculate the PYTHONPATH entries for python actions and sensors, the plugin has to extract the
entry_point
from the individual metadata files. But, thepack_metadata
target generated genericresource
targets that didn't make it easy to distinguish action metadata frompack.yaml
and other files. So, this PR changespack_metadata
so it generates apack_content_resource
target for each file instead of just using the genericresource
target. Note that thepack_content_resource
target should not actually show up in any BUILD files. It should only be generated bypack_metadata
.This is where the
pack_metadata
target says which target should be generated for each file:st2/pants-plugins/pack_metadata/target_types.py
Line 167 in b3d8c4c
This PR adds the
pack_content_resource
target here:st2/pants-plugins/pack_metadata/target_types.py
Lines 148 to 156 in b3d8c4c
It has the same fields as
resource
target with a couple additions. It extendsResourceTarget
so that anything that needs a generic resource can still get the file. Part of that is extending theResourceSourceField
because targets are often identified by the their fields not by the target itself.st2/pants-plugins/pack_metadata/target_types.py
Lines 111 to 112 in b3d8c4c
The other new field is
type
, defined here. The possible values are defined inPackContentResourceTypes
which is an enum. In nearly all cases, the BUILD files will not have an explicit type for any of these targets, so thecompute_value
method will getraw_value=None
. It then uses a map to calculate the actual value fortype
based on the file's path (inaddress
).st2/pants-plugins/pack_metadata/target_types.py
Lines 71 to 84 in b3d8c4c
If a pack has files that do not follow conventions, this PR adds an escape hatch for overriding the calculated type. The
pack_metadata
target now has theResourcesOverridesField
to provide this escape hatch. It would be used like this:The conventions for types of metadata files in subdirectories are defined here:
st2/pants-plugins/pack_metadata/target_types.py
Lines 59 to 68 in b3d8c4c
If a file doesn't follow any of the conventions, it gets
type="unknown"
, which can be overridden with the overrides field (shown above).st2/pants-plugins/pack_metadata/target_types.py
Line 108 in b3d8c4c
None of the packs in the st2 repo break with convention, except for maybe some insignificant things in fixture packs. So, I didn't have to add any
overrides
to get pack and unit tests passing.New Field:
python_tests(inject_pack_python_path=...)
This PR also has to provide a way for
python_tests
targets to opt-in to the PYTHONPATH manipulation. For example, thelinux
pack's BUILD file uses__defaults__
to enable this for all tests in the pack:st2/contrib/linux/BUILD
Line 1 in b3d8c4c
I added this by hand, instead of making
pants tailor ::
add it, because it's not needed for fixture packs. So, I added it only for the main packs that might actually get installed.This new field is added to
python_tests
andpython_test
targets here:st2/pants-plugins/pack_metadata/register.py
Lines 37 to 40 in b3d8c4c
And the field is defined as a simple Boolean field here:
st2/pants-plugins/pack_metadata/target_types.py
Lines 207 to 215 in b3d8c4c
Rules
This PR adds the actual PYTHONPATH manipulation logic in 3 sets of rules, with each set in a different file:
pack_metadata/python_rules/python_pack_content.py
pack_metadata/python_rules/python_module_mapper.py
pack_metadata/python_rules/python_path_rules.py
Note: This PR also adds tests for each of these rules.
I added a long comment that provides an overview of the implementation and how these rules work together:
st2/pants-plugins/pack_metadata/python_rules/python_pack_content.py
Lines 51 to 86 in b3d8c4c
python_pack_content
The rules in this file are responsible for finding pack content using StackStorm's conventions. Any rules that need to extract something pack metadata files should be in this file (like the rule that extracts
entry_point
from action and sensor metadata files).st2/pants-plugins/pack_metadata/python_rules/python_pack_content.py
Lines 98 to 104 in b3d8c4c
st2/pants-plugins/pack_metadata/python_rules/python_pack_content.py
Lines 178 to 181 in b3d8c4c
st2/pants-plugins/pack_metadata/python_rules/python_pack_content.py
Lines 294 to 299 in b3d8c4c
python_module_mapper
This file has a rule to inject StackStorm's custom python module lookup logic into the pants dependency inference, since the standard python import rules cannot identify pack python files. The dependency inference logic influences which files are considered dependencies of other files, and therefore which files need to be in the sandbox when running pytest and other tools on that file.
st2/pants-plugins/pack_metadata/python_rules/python_module_mapper.py
Lines 42 to 48 in b3d8c4c
python_path_rules
This file has rules to calculate StackStorm's custom pack PYTHONPATH and then inject that (as PEX_EXTRA_SYS_PATH) when pants runs pytest.
st2/pants-plugins/pack_metadata/python_rules/python_path_rules.py
Lines 48 to 54 in b3d8c4c
st2/pants-plugins/pack_metadata/python_rules/python_path_rules.py
Lines 111 to 117 in b3d8c4c
Eventually, I hope to add this PYTHONPATH manipulation for pylint as well (and possibly other utils python runs), but that will require changing pants itself to support that. Since this only affected one file (a file we don't even lint in the soon-to-be-legacy Makefile), I just skipped the pylint error:
st2/contrib/examples/actions/pythonactions/isprime.py
Lines 18 to 19 in b3d8c4c