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.

Sign up for GitHub

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

Jump to bottom

fix accidentally double-registering setuptools for ipex #9341

Merged
merged 3 commits into from
Mar 31, 2020
Merged

fix accidentally double-registering setuptools for ipex #9341

merged 3 commits into from
Mar 31, 2020

Conversation

cosmicexplorer
Copy link
Contributor

Problem

When creating .ipex files as per #8793, we don't add resolved distributions to self._distributions in PexBuilderWrapper. This leads to the annoying _sanitize_requirements() hack introduced into ipex_launcher.py in that PR (having to remove duplicate setuptools requirements).

Solution

  • Add all resolved distributions to self._distributions in PexBuilderWrapper, even when generate_ipex=True.
  • Remove the separate list of transitively-resolved requirements passed through several methods in PexBuilderWrapper.

@stuhood
Copy link
Member

stuhood commented Mar 19, 2020

It looks like there is one failure in a unit test that is potentially related.

Is this covered by existing tests?

@cosmicexplorer cosmicexplorer force-pushed the ipex-fix-double-registering-setuptools branch from 2edc355 to 588edaa Compare March 20, 2020 03:37
@cosmicexplorer
Copy link
Contributor Author

Is this covered by existing tests?

The test_generate_ipex_tensorflow() integration test passing, after removing the _sanitize_requirements() hack in ipex_launcher.py, verifies that the fix works, because if not, the pex would fail to load, since tensorflow brings in a setuptools wheel along with it that would clash otherwise.

@cosmicexplorer cosmicexplorer force-pushed the ipex-fix-double-registering-setuptools branch 4 times, most recently from 7c4a34e to c544d63 Compare March 21, 2020 01:57
@stuhood stuhood added this to the 1.25.x milestone Mar 30, 2020
@cosmicexplorer cosmicexplorer force-pushed the ipex-fix-double-registering-setuptools branch from c544d63 to b913d76 Compare March 30, 2020 22:24
@cosmicexplorer cosmicexplorer force-pushed the ipex-fix-double-registering-setuptools branch from 0bedee0 to 8d644b0 Compare March 31, 2020 04:32
@cosmicexplorer cosmicexplorer merged commit 14b1b27 into pantsbuild:master Mar 31, 2020
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 31, 2020
@cosmicexplorer
fix accidentally double-registering setuptools for ipex (pantsbuild#9341
6fe02c3 
)

When creating `.ipex` files as per pantsbuild#8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 31, 2020
@cosmicexplorer
fix accidentally double-registering setuptools for ipex (pantsbuild#9341
1647e82 
)

When creating `.ipex` files as per pantsbuild#8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 31, 2020
@cosmicexplorer
fix accidentally double-registering setuptools for ipex (pantsbuild#9341
df21e1f 
)

When creating `.ipex` files as per pantsbuild#8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
stuhood pushed a commit that referenced this pull request Mar 31, 2020
@cosmicexplorer @stuhood
fix accidentally double-registering setuptools for ipex (#9341)
a873ca1 
When creating `.ipex` files as per #8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
stuhood pushed a commit that referenced this pull request Mar 31, 2020
@cosmicexplorer @stuhood
fix accidentally double-registering setuptools for ipex (#9341)
08f051f 
When creating `.ipex` files as per #8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
@stuhood stuhood modified the milestones: 1.25.x, 1.26.x Apr 1, 2020
stuhood pushed a commit that referenced this pull request Apr 1, 2020
@cosmicexplorer @stuhood
fix accidentally double-registering setuptools for ipex (#9341)
81d399d 
### Problem

When creating `.ipex` files as per #8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

### Solution

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
cosmicexplorer added a commit that referenced this pull request May 4, 2020
@cosmicexplorer
fix accidentally double-registering setuptools for ipex (#9341)
3ba012b 
When creating `.ipex` files as per #8793, we don't add resolved distributions to `self._distributions` in `PexBuilderWrapper`. This leads to the annoying `_sanitize_requirements()` hack introduced into `ipex_launcher.py` in that PR (having to remove duplicate `setuptools` requirements).

- Add all resolved distributions to `self._distributions` in `PexBuilderWrapper`, even when `generate_ipex=True`.
- Remove the separate list of transitively-resolved requirements passed through several methods in `PexBuilderWrapper`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eric-Arellano Eric-Arellano Eric-Arellano left review comments

@wisechengyi wisechengyi wisechengyi approved these changes

@stuhood stuhood Awaiting requested review from stuhood

@jsirois jsirois Awaiting requested review from jsirois

@benjyw benjyw Awaiting requested review from benjyw

@hrfuller hrfuller Awaiting requested review from hrfuller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.26.x
Development

Successfully merging this pull request may close these issues.
4 participants
@cosmicexplorer @stuhood @wisechengyi @Eric-Arellano