Vendoring a package via |mach vendor python| also updates all transient dependencies

RESOLVED FIXED in Firefox 64

Status

enhancement
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

({in-triage})

unspecified
mozilla64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments)

Direct dependencies are added to Pipfile and pinned to a specific version, however transient dependencies are only pinned in the Pipfile.lock when the full dependency graph is determined. When a new or updated package is vendored, pipenv regenerates the Pipfile.lock, which means the transient dependencies are updated to the latest compatible versions. Ideally, we would limit changes to only the packages we're (re)vendoring and any direct transient dependencies.

Unfortunately this is not currently supported in pipenv, however many users are asking for it. See https://github.com/pypa/pipenv/issues/966 and https://github.com/pypa/pipenv/issues/1554 for related discussions in the pipenv issue tracker.

A recent comment from a pipenv contributor:
> We have not forgotten or ignored this issue, we have a full implementation of a 
> fix in the resolver linked above. Have patience, be courteous, or find somewhere 
> else to talk. To those who have been waiting patiently for a fix, please do try 
> the resolver mentioned above— we are eager to see if it meets your needs. It 
> implements proper backtracking and resolution and shouldn’t handle this upgrade 
> strategy.

The resolver mentioned is https://github.com/sarugaku/passa and it may be worth investigating as an alternative to pipenv for the vendoring, however it sounds like pipenv may eventually use this new resolver anyway.
I've noticed this while vendoring a few packages and it's definitely annoying. If upstream pipenv doesn't get to this soon we may want to look into using passa as a workaround.
:davehunt

Is this something you intend to investigate or are you looking for assistance from the build team?  irrc you had some earlier bugs that you were working on in our bucket so I thought I'd ask
Flags: needinfo?(dave.hunt)

Updated

9 months ago
Keywords: in-triage
(In reply to Kim Moir [:kmoir] ET from comment #2)
> :davehunt
> 
> Is this something you intend to investigate or are you looking for
> assistance from the build team?  irrc you had some earlier bugs that you
> were working on in our bucket so I thought I'd ask

I'm not likely to be able to spend much time on this in the next couple of weeks. I'd be happy to mentor or review a patch trying out using passa as an alternative resolver though.
Flags: needinfo?(dave.hunt)
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Instead of trying passa, I decided to use pip-tools, which is more mature. This will move us away from Pipfile/Pipfile.lock for vendoring, but will still allow us to use a requirements.txt as a lock file with the full dependency graph resolved and with hashes. I've pushed a patch for review.
The patch looks good, though I think this might warrant some more discussion before landing. A few downsides to your patch:

1) We now use both pipenv and pip-tools for various purposes, adding complexity. We also depend on pipenv to run our python-tests with python 3, so I'm not sure we'll be able to completely remove it.

2) Pipfile/Pipfile.lock seem to be the standard that pip is migrating to. So we're moving away to something non-standard.

3) We lose extra features that pipenv has, like the ability to specify which pypi to download a package from.

I'd like to know your long term vision. Is this meant to be a temporary switch away from pipenv until things improve or are officially supported in pip? Is this switch *only* going to happen for |mach vendor python| and other (current and future) uses will continue to use pipenv?

I'm not saying that we shouldn't do this. I just feel uneasy giving an r+ without more context/deeper consideration.
Flags: needinfo?(dave.hunt)
After digging into poetry and reading comparison articles, it looks like an absolute gong show out there. There are multiple competing formats (Pipfile, pyproject.toml and requirements.in) at least two of which are on standards tracks, then there are multiple libraries that implement each of them.

I think I agree with your decision to stick to pip-tools for now. It seems to do the least amount of things (just handles dependency pinning and nothing else), which is a big plus for our monorepo edge case. The passa library looks promising, but I agree that it is too new to be used in production. Maybe if Pipfile ever gets official adoption we can switch to it in the future.

I'd still like to hear your overall thoughts and long term vision though.
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> I'd still like to hear your overall thoughts and long term vision though.

It's hard to give long term vision when like you say, there are several competing formats, and each claim to be better than others for reasons.

We're currently using pipenv for vendoring packages, where the main benefit is that we resolve the entire dependency graph and generate a lock file. There are two main issues with using pipenv here. First, transient dependencies are updated whenever a new package is vendored, or a vendored package is updated. This issue has been acknowledged, and it would seem that passa is the work in progress solution. Another issue is that the transient requirements.txt that pipenv generates does not include hashes so there is a chance we could download a modified pacakge if the index has been compromised. I learned today that pipenv itself uses a patched version of pip-tools for resolving the dependencies, but despite that, using pip-tools directly addresses both of these issues. For vendoring, I believe all we need is a resolver, and that using pipenv is overkill. Possible future changes here might be to switch to passa if there is a compelling reason to use Pipfile, or to use a future version of pip that allows us to generate a full dependency list.

We're also using pipenv to run the Python tests against multiple versions of Python. Initially I was hoping that we'd use pipenv to take care of managing our virtual environments, however I discovered that I needed to scale this back due to the number of modules we use that aren't packaged. We're still using pipenv to manage the creation of virtual environments, but we populate them using the same approach we use elsewhere. Towards the end of getting this set up, I was coming to the conclusion that pipenv was likely to be scaffolding that gives us the ability to run tests against Python 3, but that really all we're using is pipenv's discovery of Python binaries, and the virtual environment initialisation. In the future I can see us augmenting our existing virtual environment manager to support discovery of Python versions, and removing the dependency on pipenv.

The final place that we're using pipenv is for mach commands that define their own Pipfiles, and we're using pipenv to create the virtual environment, install the dependencies, and run the command(s). I know we've at least done this for generating the documentation. This is where the features of pipenv are most utilised, and think it could be a good practice to promote. That said, if pip at some point supports a lock file format such as Pipfile.lock, then I could see an opportunity to remove the dependency on pipenv.
Flags: needinfo?(dave.hunt)
Thanks for the summary!

Comment 12

8 months ago
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/376207f84aa3
Vendor pip-tools 3.0.0 and dependencies; r=ahal

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/376207f84aa3
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Looks like only the first patch landed so far.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 15

8 months ago
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f92dcf5319ae
Replace pipenv with pip-tools for vendoring packages and dependencies; r=ahal
Backed out for test_mozbuild_reading.py bustages

backout: https://hg.mozilla.org/integration/autoland/rev/be3c71747eff927b12bf9a79e8b57331931ef115


push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=204336717&revision=f92dcf5319ae2b9f1a305c0e155c463584c5fdf2

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204336714&repo=autoland&lineNumber=41457

[task 2018-10-09T21:46:51.389Z] 21:46:51     INFO - ============================= test session starts ==============================
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - platform linux2 -- Python 2.7.9, pytest-3.6.2, py-1.5.4, pluggy-0.6.0 -- /builds/worker/workspace/build/src/obj-firefox/_virtualenvs/obj-firefox-8yIyzR8r-2.7/bin/python
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - rootdir: /builds/worker/workspace/build/src, inifile: /builds/worker/workspace/build/src/config/mozunit/mozunit/pytest.ini
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - collecting ... collected 3 items
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - ../config/tests/test_mozbuild_reading.py::TestMozbuildReading::test_filesystem_traversal_no_config PASSED
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - ../config/tests/test_mozbuild_reading.py::TestMozbuildReading::test_filesystem_traversal_reading <- ../../../../../usr/lib/python2.7/unittest/case.py SKIPPED
[task 2018-10-09T21:46:51.390Z] 21:46:51  WARNING - ../config/tests/test_mozbuild_reading.py::TestMozbuildReading::test_orphan_file_patterns TEST-UNEXPECTED-FAIL
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - =================================== FAILURES ===================================
[task 2018-10-09T21:46:51.390Z] 21:46:51     INFO - ________________ TestMozbuildReading.test_orphan_file_patterns _________________
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO - self = <test_mozbuild_reading.TestMozbuildReading testMethod=test_orphan_file_patterns>
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -     def test_orphan_file_patterns(self):
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -         if sys.platform == 'win32':
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -             raise unittest.SkipTest('failing on windows builds')
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -         mb = MozbuildObject.from_environment(detect_virtualenv_mozinfo=False)
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -         try:
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -             config = mb.config_environment
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -         except Exception as e:
[task 2018-10-09T21:46:51.391Z] 21:46:51     INFO -             if e.message == 'config.status not available. Run configure.':
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -                 raise unittest.SkipTest('failing without config.status')
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -             raise
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         if config.substs['MOZ_BUILD_APP'] == 'js':
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -             raise unittest.SkipTest('failing in Spidermonkey builds')
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         reader = BuildReader(config)
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         all_paths = self._mozbuilds(reader)
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         _, contexts = reader.read_relevant_mozbuilds(all_paths)
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         finder = FileFinder(config.topsrcdir, ignore=['obj*'])
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         def pattern_exists(pat):
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -             return [p for p in finder.find(pat)] != []
[task 2018-10-09T21:46:51.392Z] 21:46:51     INFO -         for ctx in contexts:
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -             if not isinstance(ctx, Files):
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -                 continue
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -             relsrcdir = ctx.relsrcdir
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -             for p in ctx.patterns:
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -                 if not pattern_exists(os.path.join(relsrcdir, p)):
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -                     self.fail("The pattern '%s' in a Files() entry in "
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -                               "'%s' corresponds to no files in the tree.\n"
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO -                               "Please update this entry." %
[task 2018-10-09T21:46:51.393Z] 21:46:51     INFO - >                             (p, ctx.main_path))
[task 2018-10-09T21:46:51.394Z] 21:46:51     INFO - E                   AssertionError: The pattern 'Pipfile*' in a Files() entry in '/builds/worker/workspace/build/src/moz.build' corresponds to no files in the tree.
[task 2018-10-09T21:46:51.394Z] 21:46:51     INFO - E                   Please update this entry.
Flags: needinfo?(dave.hunt)

Comment 17

8 months ago
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fead446e3b9
Replace pipenv with pip-tools for vendoring packages and dependencies; r=ahal

Comment 19

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fead446e3b9
Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED

Comment 20

8 months ago
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ccf0f4b0ef2
Update documentation on vendoring Python packages based on switch to pip-tools; r=ahal
Flags: needinfo?(dave.hunt)
You need to log in before you can comment on or make changes to this bug.