Skip to content
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 'current' platform check for python_dist() targets with C/C++ sources #7687

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

commented May 9, 2019

Problem

Currently, in BuildLocalPythonDistributions, any python_dist() targets which depend on shared libs we compile and link from ctypes_compatible_cpp_library() targets are converted into platform-specific wheels, with the platform matching the current host (this is then injected into the build graph as a PythonRequirementLibrary). In ResolveRequirementsTaskBase, we attempt to check for whether any such python_dist() targets with native code exist, and if so, ensure we only resolve for the platform ['current']. This check is currently broken in such a way that we never find any native code, and we always try to resolve for all the platforms in --python-setup-platforms (or whatever platforms may be set on individual python_binary() targets). If --python-setup-platforms has anything besides ['current'], this resolve will fail, because we've only built our distributions with native code locally, i.e. for the current platform.

This wasn't noticed until #7618, which ended up essentially assigning --python-setup-platforms to python_tests() targets, causing the resolve to fail in our repo when running tests. We also then realized that we don't actually ever need to resolve for anything but the 'current' platform except explicitly in PythonBinaryCreate, so we were able to remove all of this logic from ResolveRequirementsTaskBase and only trigger the check when it actually needs to fail.

Solution

  • Don't check the platforms and always resolve for 'current' in ResolveRequirementsTaskBase#resolve_requirements().
  • Work around the issue catalyzed by #7618 by not assigning any platforms to python_tests() when platform checking unless the issue in #7616 is encountered specifically.
  • Fix the broken platform check.
  • Do a very small refactor of some of the python_dist() testing.

Result

The platform checking in ResolveRequirementsTaskBase is removed entirely, only occurring during python binary creation instead. When that platform checking does occur, python_tests() will only be assigned platforms if another target has an explicit platforms setting, and python_dist() targets with native code will be successfully resolved by the ResolveRequirements task.

@cosmicexplorer cosmicexplorer added this to the 1.16.x milestone May 9, 2019

@cosmicexplorer cosmicexplorer requested review from stuhood, benjyw, illicitonion, ity, CMLivingston and Eric-Arellano and removed request for ity May 9, 2019

@illicitonion
Copy link
Contributor

left a comment

The first half looks reasonable. I'm not sold on the flag. It feels like we should either:

  1. For things with native deps: ignore the python_setup platforms value, error if you specify anything other than platforms = ['current'] on the target, and otherwise act as if python_setup specified platforms = ['current']
  2. Hard error and force you to explicitly specify platforms = ['current'] on any targets with native deps, if your python_setup platforms are other than that.

It feels like the flag is trying to do both, but I'm not sure why we want to make it configurable?

@cosmicexplorer cosmicexplorer changed the title Fix 'current' platform check for python_dist() targets with C/C++ sources [WIP] Fix 'current' platform check for python_dist() targets with C/C++ sources May 9, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Ok, I will employ one or both of those approaches. I think removing the flag is great and wasn't sure how to do that before. Will ping for re-review.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-current-platform-only-check-failure-for-python-dist branch 2 times, most recently from 1a8bc51 to 262715c May 10, 2019

@cosmicexplorer cosmicexplorer changed the title [WIP] Fix 'current' platform check for python_dist() targets with C/C++ sources Fix 'current' platform check for python_dist() targets with C/C++ sources May 10, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Ok, I've implemented:

For things with native deps: ignore the python_setup platforms value, error if you specify anything other than platforms = ['current'] on the target, and otherwise act as if python_setup specified platforms = ['current']

This approach also fixes the failure we were seeing in another repo using pants.

@cosmicexplorer cosmicexplorer requested a review from illicitonion May 10, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Ok, this is probably ready for re-review now.

@Eric-Arellano
Copy link
Contributor

left a comment

Looks good!

Biggest request is to use kwargs for tests.

data_files=[('', ['libc-math-lib.so'])],
)
""",
'setup.py': dedent("""\

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Contributor

Bless you for adding dedent :D

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

In the future, if you can do this in a separate PR (which is nicely trivial to review), it would help to reduce the noise in this review (and adds the benefit that reverts and merges can be better targeted) :)

platforms=['current', 'linux-x86_64', 'macosx_10_14_x86_64'])
platform_specific_dist = self.target_dict['platform_specific_ctypes_cpp_dist']
self._assert_dist_and_wheel_identity(
'platform_specific_ctypes_cpp_dist', '0.0.0', self.ExpectedPlatformType.current,

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Contributor

I find these function calls a bit difficult to quickly know what each value is. I recommend using kwargs for everything.

This comment has been minimized.

Copy link
@illicitonion

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 10, 2019

Author Contributor

Will do!

@illicitonion
Copy link
Contributor

left a comment

Looks good :) Thanks for the clean up!

'options are compatible with the current platform. Found targets for platforms: {}'
.format(str(platforms_with_sources)))
raise IncompatiblePlatformsError(dedent("""\
The target set contains one or more targets that depend on native code. Please ensure that the

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

Let's be more specific in this error. What I'm hoping we can get to is roughly:

Pants doesn't currently support cross-compiling native code.
The following targets set platforms arguments other than ['current'], which is unsupported for this reason.
Please either remove the platforms argument from those targets, or set them to exactly ['current'].
Bad targets:
{}

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor

Also perhaps consider the recent change to remove this check being performed anywhere except in PythonBinaryCreate -- that might allow us to reduce the number of non-local exit paths in this method?

# BuildLocalPythonDistributions depend on the C/C++ sources that it builds might make this
# cleaner.
all_targets = self.get_targets()
if self._python_native_code_settings.check_build_for_current_platform_only(all_targets):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

I'm slightly concerned that in the case that some things being built have native deps, and some don't, we'll only build for the current platform for all of those targets.

But I guess this is probably an ok trade-off until such time as we do per-root resolves. We need to pick one way or the other, so...

data_files=[('', ['libc-math-lib.so'])],
)
""",
'setup.py': dedent("""\

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

In the future, if you can do this in a separate PR (which is nicely trivial to review), it would help to reduce the noise in this review (and adds the benefit that reverts and merges can be better targeted) :)

platforms=['current', 'linux-x86_64', 'macosx_10_14_x86_64'])
platform_specific_dist = self.target_dict['platform_specific_ctypes_cpp_dist']
self._assert_dist_and_wheel_identity(
'platform_specific_ctypes_cpp_dist', '0.0.0', self.ExpectedPlatformType.current,

This comment has been minimized.

Copy link
@illicitonion

def test_resolve_for_native_sources_allows_current_platform_only(self):
platform_specific_dist = self.target_dict['platform_specific_ctypes_cpp_dist']
compatible_python_binary_target = self.make_target(

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

For the future (not important now), make_target is a kind of legacy API that doesn't work well with v2-ifying things; the suggested replacement is to call self.add_to_build_file and then self.target(spec)

with self.assertRaisesWithMessage(IncompatiblePlatformsError, dedent("""\
The target set contains one or more targets that depend on native code. Please ensure that the
platform arguments in python_binary() targets, as well as the value of
--python-setup-platforms, are compatible with the current platform.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

--python-setup-platforms is no longer relevant to this check; should be removed from the error

The target set contains one or more targets that depend on native code. Please ensure that the
platform arguments in python_binary() targets, as well as the value of
--python-setup-platforms, are compatible with the current platform.
Platform assignments for python targets: {'current': OrderedSet([PythonBinary(src/python/plat_specific:bin)]), 'linux-x86_64': OrderedSet([PythonBinary(src/python/plat_specific:bin)]), 'macosx_10_14_x86_64': OrderedSet([PythonBinary(src/python/plat_specific:bin)])}

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

The dict + OrderedSet + PythonBinary representation here isn't very user-friendly. Ideally this would read more like:
Target src/python/plat_specific:bin has unsupported platforms linux-x86_64 and macosx_10_14_x86_64

(and ideally we would resolve current before doing this check, so that we know that current == linux or macosx)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor
from pex import pep425tags
print(pep425tags.get_platform())

is how to resolve the current platform, if we want that.

@@ -98,6 +100,9 @@ def _create_distribution_synthetic_target(self, python_dist_target, extra_target

return context, synthetic_target, snapshot_version

class ExpectedPlatformType(enum(['universal', 'current'])):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 10, 2019

Contributor

I prefer 'any' to 'universal' here - there doesn't seem to be a strong reason for us to introduce conflicting terminology, and if it's always just any, people don't need to think about whether where they're writing code is in a "pants" context ("universal") or a "python" context ("any")

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 11, 2019

Author Contributor

any was highlighted in my text editor as a keyword when accessed with ExpectedPlatformType.any is the only reason I decided to change this. I'm not sure whether instantiating it with ExpectedPlatformType('any') is appropriate or if the intent is clear enough that ExpectedPlatformType.any is allowed. cc @Eric-Arellano

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Contributor

ExpectedPlatformType.any should be fine; I think your text editor is probably being buggy here. any is a global function, but it's clearly not a reference to the global function in this position - we use other global function names in this kind of place, like compile, exec, and such. any is not a keyword in the language:

$ python -c 'import keyword ; import sys ; print (sys.version_info) ; print(keyword.iskeyword("any"))'
sys.version_info(major=2, minor=7, micro=10, releaselevel='final', serial=0)
False
$ python3 -c 'import keyword ; import sys ; print (sys.version_info) ; print(keyword.iskeyword("any"))'
sys.version_info(major=3, minor=6, micro=4, releaselevel='final', serial=0)
False

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 13, 2019

Contributor

The only thing that is strongly discouraged is a bare any, because it will shadow the builtin for that file and means that no where in that scope can we use any.

So, we should not do either of these:

def any():
  pass

any = True

These things are okay though:

platform = "any"  # quotes make it a string

ExpectedPlatformType.any  # attribute is safe for the caller (likely not for the original class)

So, this is all to say that yes, here we can use any, because the value is specified in the enum as a string so we never actually end up shadowing the builtin.

illicitonion added a commit that referenced this pull request May 14, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-current-platform-only-check-failure-for-python-dist branch from 262715c to cf2eea2 May 14, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I've just pushed a change which replaces some of the more complex changes in ResolveRequirementsTaskBase#resolve_requirements() with platforms = ['current'] after realizing that PythonBinaryCreate explicitly provides the platforms it resolves for (and calls .check_build_for_current_platform_only()), so we don't have to do that again ourselves.

This means the IncompatiblePlatformsError should only show up when PythonBinaryCreate is run, not for other tasks such as tests which (right now) will definitely only require dependencies resolved for the current host platform. As a result, I've skipped a unit test which I believe should be migrated to another class testing PythonBinaryCreate (likely in the same test directory) to verify the failure occurs. This change also fixes the error we were seeing internally, which was the subject of the skipped unit test. Now that non-'current' platforms are only checked in PythonBinaryCreate, we don't fail when running python_tests() which depend on a python_dist() with native code.

This test could probably be created by creating a new class in test_ctypes.py subclassing BuildLocalDistributionsTestBase, overriding run_before_task_types to append BuildLocalPythonDistributions, and overriding task_type to return PythonBinaryCreate. This would allow using self.invoke_tasks() from DeclarativeTaskTestMixin to invoke all the necessary tasks -- but that's just one way to do it. I would want this test to be added before merging as that will be what is testing that we're doing this check correctly.

Going to be handing this off to @illicitonion, whom I thank immensely for diving into this.

for target in targets_requiring_default_platforms:
for platform in python_setup.platforms:
explicit_platform_settings[platform].add(target)
return dict(explicit_platform_settings)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor

I believe this PR should work without this change, although I could make an argument that this is more correct, but I'm really not sure, and this is such a big change to the file I would probably be ok either completely reverting it or keeping it as long as there's a success/failure test for platform resolution (the skipped unit test, basically).

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 15, 2019

Contributor

I find the new version much easier to understand and think that is more correct.

@@ -57,10 +57,6 @@ def __init__(self,
def has_native_sources(self):
return self.has_sources(extension=tuple(self.native_source_extensions))

@property
def platforms(self):
return ['current']

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor

Note that this is part of removing PythonDistribution from may_have_explicit_python_platform() in pex_build_util.py.

req = PythonRequirement(req_name, repository=whl_dir)
name, version = base.split('-')[0:2]
full_req_string = '{}=={}'.format(name, version)
req = PythonRequirement(full_req_string, repository=whl_dir)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor

Cosmetic changes, can be reverted.

@@ -150,7 +150,7 @@ def _create_binary(self, binary_tgt, results_dir):

# We need to ensure that we are resolving for only the current platform if we are
# including local python dist targets that have native extensions.
self._python_native_code_settings.check_build_for_current_platform_only(self.context.targets())
self._python_native_code_settings.check_build_for_current_platform_only(self.get_targets())

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 14, 2019

Author Contributor

This shouldn't change any logic at all.

@stuhood
Copy link
Member

left a comment

Thank you!

Does the description need an update based on the latest changes?

platforms = ['current']
else:
platforms = list(sorted(targets_by_platform.keys()))
# NB: Since PythonBinaryCreate is the only task that exports python code for use outside the

This comment has been minimized.

Copy link
@stuhood

stuhood May 14, 2019

Member

👍

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Does the description need an update based on the latest changes?

Tried to do that just now!

@illicitonion illicitonion force-pushed the cosmicexplorer:fix-current-platform-only-check-failure-for-python-dist branch from cf2eea2 to 67e0aae May 15, 2019

illicitonion added some commits May 15, 2019

Remove obsolete test
ResolveRequirements is now hard-coded to be single-platform
for target in targets_requiring_default_platforms:
for platform in python_setup.platforms:
explicit_platform_settings[platform].add(target)
return dict(explicit_platform_settings)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 15, 2019

Contributor

I find the new version much easier to understand and think that is more correct.

bad_targets = set()
for platform, targets in platforms_with_sources.items():
if platform == 'current':
continue

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 15, 2019

Contributor

Great! I think an early return like this makes it much easier to follow what is the main logic in this structure, rather than having it nested.

Show resolved Hide resolved ...thon/pants_test/backend/python/tasks/util/build_local_dists_test_base.py Outdated
Bad targets:
{}
""".format('\n'.join(sorted(target.address.reference() for target in bad_targets)))
))

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 15, 2019

Author Contributor

This is much much better (displaying just the target name instead of trying to keep the error message updated with all the ways that platforms can be assigned to targets).

@illicitonion illicitonion merged commit 096b135 into pantsbuild:master May 15, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

illicitonion added a commit that referenced this pull request May 15, 2019

Fix 'current' platform check for python_dist() targets with C/C++ sou…
…rces (#7687)

### Problem

Currently, in BuildLocalPythonDistributions, any `python_dist()` targets which depend on shared libs we compile and link from `ctypes_compatible_cpp_library()` targets are converted into platform-specific wheels, with the platform matching the current host (this is then injected into the build graph as a PythonRequirementLibrary). In ResolveRequirementsTaskBase, we attempt to check for whether any such `python_dist()` targets with native code exist, and if so, ensure we only resolve for the platform `['current']`. This check is currently broken in such a way that we never find any native code, and we always try to resolve for all the platforms in `--python-setup-platforms` (or whatever platforms may be set on individual `python_binary()` targets). If `--python-setup-platforms` has anything besides `['current']`, this resolve will fail, because we've only built our distributions with native code locally, i.e. for the current platform.

This wasn't noticed until #7618, which ended up essentially assigning `--python-setup-platforms` to `python_tests()` targets, causing the resolve to fail in our repo when running tests. We also then realized that we don't actually ever need to resolve for anything but the `'current'` platform *except* explicitly in `PythonBinaryCreate`, so we were able to remove all of this logic from `ResolveRequirementsTaskBase` and only trigger the check when it actually needs to fail.

### Solution

- Don't check the platforms and always resolve for `'current'` in `ResolveRequirementsTaskBase#resolve_requirements()`.
- Work around the issue catalyzed by #7618 by not assigning any platforms to `python_tests()` when platform checking unless the issue in #7616 is encountered specifically.
- Fix the broken platform check.
- Do a very small refactor of some of the `python_dist()` testing.

### Result

The platform checking in ResolveRequirementsTaskBase is removed entirely, only occurring during python binary creation instead. When that platform checking does occur, `python_tests()` will only be assigned platforms if another target has an explicit `platforms` setting, and `python_dist()` targets with native code will be successfully resolved by the `ResolveRequirements` task.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.