feat: improve requirements file structure

[aht007]

Description

This PR updates the structure of requirements file. With the newer approach, dependencies will only appear in corresponding layer file and not repeated in multiple file. Each requirements file will have just the dependencies needed for the packages listed in the file currently being processed, but at versions compatible with the previous layer of dependencies.

What we hope to gain from this change are the following:

1. Faster make upgrade performance by not having to process as many dependencies for each layer

2. Better ease of reviewing dependency updates, since most dependency versions will only appear in a single requirements file, whereas many currently appear in 3 or more.

Issue: https://2u-internal.atlassian.net/browse/BOM-2483

[aht007]

@timmc-edx I can see that you made this change just recently in this PR but it is causing test failures for me with the updated requirements file structure. I don't exactly know why this behavior but when installing dependencies with update requirements files and pip-sync, unit tests fail to import certain dependencies which are required by our own packages listed in github.in requirements file.
Example output of failed import: Logs
I can also see this package being uninstalled here: Logs
The only reason I could come up with is that, with the updated requirements file structure, these dependencies never end up in the requirements file as they are generated on run time using pip and hence pip-sync not finding these dependencies explicitly written in the requirements file decides to uninstall them while never installing them back.
Looking for a cent or two from your side.

[timmc-edx]

Hmm, I can take a look and see if I can figure out what's going on. But overall I think we have to pick between two options:

• Always delete the virtualenv and any other installation locations, followed by pip install -r, or
• Use pip-sync

If we don't do one of those, we have non-repeatable builds, which reduces our confidence in CI and creates debugging nightmares.

[timmc-edx]

OK, so the issue here is that this PR is changing testing.txt so that it is no longer a pip-compile output—and pip-sync is only designed to work with the output of pip-compile. Really, testing.txt is structured as an .in file. It pulls in github.in and via that gets the blockstore dependency, but it never includes blockstore's own dependencies (such as pyblake2).

One approach would be to mv testing.txt testing.in and add requirements/edx/testing to the REQ_FILES list. I suspect that this does not get you the performance gains you were hoping for, though.

Another approach would be to build all the layers, but not have a testing.txt to represent the combination. Instead, call pip-sync base.txt coverage_layer.txt testing_layer.txt (although at this point you could drop the _layer suffixes). pip-sync can combine the layers for you.

[aht007]

@timmc-edx I would prefer the second approach, but I am tempted to retain the the current structure and the layer suffix as well. The reason being that this workaround pip-sync base.txt coverage_layer.txt testing_layer.txt would be for our CI whereas I would still like to keep the testing.txt file to give pointer to other people about what requirements they need to install for testing purposes and if they aren't using pip-sync they can always just pip install the testing.txt file. Looking for your take on this.

[aht007]

@timmc-edx using pip-sync base.txt coverage_layer.txt testing_layer.txt didn't work and resulted in the same error as base.txt is also not the output of pip_compile. I then tried using the requirement files contained in base.txt directly but it still didn't work because pip-sync complains about github.in being a .in file and not a .txt file. Simply changing file extension also doesn't make pip-sync install the dependencies like pyblake2 and using pip-compile on github.in gives some sort of conflicts with the dependencies which I tried to resolve using constraint files but to no avail.
I then came up with this workaround to run pip-compile on testing.txt and put the output in a temporary file, run pip-sync on that temporary file and then delete that temp file. It seems to work but it increased the time for requirements installation step by around 20-25 seconds.

[aht007]

@timmc-edx I had a look at this 7a94179 and lock-all approach would still make us repeat dependencies in at least 2-3 places which kind of negates what we are trying to achieve in this PR.
About combining both base.in and github.in, I don't think folks out there would be happy with that(although its the most easiest way out of this situation) since people wouldn't be able to visualize what comes from github and what comes from PyPI. Also compiling github.in to github.txt wouldn't work unless we use the lock-all thing because only that way we will come out of all the dependency conflict. Also I don't know what impact will this approach have on the performance of installing and upgrading requirements.
That being said, how does the current way of compiling the dependencies in a temporary file fair with all the approaches above here. Is the increase in time of installing requirements a big blow to the performance?

[aht007]

@timmc-edx Reminder just in case you missed the earlier comment.

[timmc-edx]

I think the right way to handle github deps in particular is the thing I mentioned at the end of my comment -- base.in includes a -r github.in line, and so they get compiled together into a single base-layer.txt file. This is probably the solution we'd want to use regardless of what overarching approach we would take with layer files, lock-all, or other ideas.

I don't understand your "current way" link -- it looks like it would involve reading from testing.txt but I don't see what would be producing that file.

On the matter of duplication and review, here's how I see things at the moment:

• We need to be able to generate lockfiles (currently pip-compile, but we don't have to stick with this necessarily)
• We need to be able to run a command that makes the current environment match a set of one or more lockfiles (currently pip-sync)
• If we're using multiple lockfiles, we encounter the problem of transitive dependencies -- there will always be something required by both base and testing that is not mentioned in either.
  • In the current state on master, we get duplication of more than this. (texting.txt includes all of base.txt)
  • I think that in the currently tried alternatives, transitive dependencies end up duplicated, but explicit dependencies do not.
  • Theoretically, we could do better, and each dependency would be present only in one lockfile, but this would come at a cost: We would have to build a deduplicator ("remove everything from testing.txt that's already in base.txt") and I don't think we want to do that. Maybe that could be a new feature for pip-tools, though, or maybe another Python dependency manager already supports this.

I think we should take a step back and reevaluate the goals and strategies. If ease of review is a concern, what if the upgrade job reported a deduplicated view of the diff? If performance is an issue, are there improvements that need to be made to pip, pip-tools, our command options (we currently clear the cache!), or something else in the toolchain?

Incidentally, I've updated the megalock branch to treat lock-all.txt as a temporary file to be deleted after compilation, since it's redundant with all the compiled layers. It still retains redundancy across layers, though. In running make compile-requirements, I found that compiling lock-all took most of the time, the layers took only about a second each since they are essentially just filtering lock-all, and then the github layer took the remaining time. For performance improvements, I wonder if there are some easier wins with the git URLs and ensuring that they're not re-fetched unnecessarily.

[aht007]

I included github.in in base_layer.in and it resolves the dependency conflicts that we were facing.
Also about your question regarding testing.txt, this is how testing.txt looks like now with upgraded structure testing.txt
By the "current way" I mean using pip-compile on the current testing.txt file and generate a temp file that will be fed to pip-sync and removed afterwards. This way we will still be using lockfile(pip-compile) and also pip-sync. The mega-lock thing becomes out of scope of the current ticket that I am working on but that is something we can look into afterwards and nevertheless I would say that it has also got my attention.
About taking a step back and reevaluating our goals and strategies, I would like to have @jmbowman's views on that.

[jmbowman]

Finally coming back to this thread after catching up on GitHub notifications. As noted in the original ticket, the primary motivations for trying this were smaller diffs and potential performance gains. It sounds like the megalock approach resolves a different problem that we occasionally hit, and probably also yields performance gains, but at the expense of even larger diffs (due to pip-sync limitations on what kinds of files we can feed it).

Some historical context on github.in: we originally created it because older versions of pip-compile choked on common VCS (Version Control System) dependency expressions. So we bypassed pip-compile for those and installed them as-is via pip. But that's since been fixed in pip-compile, and we only retained the separate file to help keep track of the backlog of VCS dependencies that we still needed to fix to install from PyPI. Recent Arbi-BOM work has greatly shrunk that file; I'm ok with just merging it into base.in, especially if we have or add a repo health check to count VCS dependencies.

Assuming I've caught up on the thread to date accurately, I'd suggest:

• Shelve the megalock approach for now, I'm not sure it would resolve problems often enough to justify the diff size penalty (even though it's otherwise a more correct approach to this)
• Merge base.in and github.in if it helps, otherwise we'll do that in a separate PR later (maybe after adding a corresponding health check)
• Skip the run_before changes to tox, given that we've since made other changes to our approach to multi-Django-version testing in build: Set Django version for tests more safely; drop support for non-GHA #31387
• Use make targets to run pip-sync with the correct sets of input files rather than using aggregation requirements files, since those don't seem to play nicely with pip-sync
• Fix anything that currently assumes the existence of test/dev level aggregated requirements files to call the appropriate make targets instead (there may be some such code in other repos like configuration)

Does that answer all the outstanding questions enough to move forward?

[aht007]

Note for reviewers.
While working on this PR, it was uncovered to me that we cannot constraint based off of a requirements file which itself references other requirements files. In case I used -c testing.txt here it would end up constraining requirements according to testing.txt but also fetch all the requirements from testing.txt explicitly into the development_layer.txt effectively repeating the requirements in multiple files and negating all the effort that we are doing in this ticket.

[e0d]

@aht007 should this be closed?

[aht007]

@aht007 should this be closed?

Closing for now. @openedx/arbi-bom can take this over and reopen when they prioritize this issue and in case they feel the need to reopen the PR.