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

Pinned dependencies getting out of sync between Pipfile and setup.py #6006

Open
jc-harrison opened this issue Mar 29, 2023 · 6 comments · May be fixed by #6014
Open

Pinned dependencies getting out of sync between Pipfile and setup.py #6006

jc-harrison opened this issue Mar 29, 2023 · 6 comments · May be fixed by #6014
Assignees
Labels
bug Something isn't working dependencies Pull requests that update a dependency file FlowAPI Issues related to the FlowKit API FlowAuth Issues related to FlowAuth FlowClient Issues related to FlowClient flowkit-jwt-generator FlowMachine Issues related to FlowMachine

Comments

@jc-harrison
Copy link
Member

Describe the bug
We are duplicating dependency constraints in the various FlowKit python packages (flowapi, flowauth, flowclient, flowkit-jwt-generator, flowmachine; no longer the case for flowetl since #5962) - constraints in setup.py and Pipfile must be manually kept in sync in each case.

This alone is an issue, since it runs the risk that if we update one but forget to update the other, the dependency versions used for testing will be different from those that would be installed via pip install flow<package>. Pipenv's recommendation when using both setup.py and Pipfile is to add the local package as an editable requirement in the Pipfile. This would be different from our current setup, though - currently a non-dev pipenv install will install all of the package dependencies but not the package itself (editable or otherwise).

It becomes more of an issue, though, because dependabot automatically bumps pinned versions in the Pipfiles without changing the corresponding versions in setup.py. Because we editable-install the package in the dev dependencies, this inconsistency leads to an un-lockable env, but for whatever reason that doesn't stop dependabot from making the change (I think because dependabot doesn't correctly handle dependencies of local-path requirements), and it still produces a pipenv install-able Pipfile.lock. I don't think this is a problem for the dev install - locked (upgraded) deps won't be overwritten by versions specified in setup.py, so unit tests will be correctly run using the dependabot-upgraded dependencies - but it is a problem in the docker images (which therefore affects some of the integration tests). In the Dockerfiles we first do a non-dev pipenv install, and then run pipenv run python setup.py install, which will overwrite the pipenv-pinned versions with versions specified in setup.py. Similarly, non-containerised integration tests will use dependencies from the local-path installs of the packages in the integration tests env, which will pick up dependencies from each package's setup.py (not from that package's Pipfile). The result is that dependabot can bump version pins in a Pipfile, and integration tests will continue to use the older versions pinned in setup.py, so Pipfile pins can be "successfully" bumped to versions that would break the integration tests.

Installing the packages without dependencies in the Dockerfiles (e.g. using pip install --no-deps . instead of python setup.py install) would avoid overwriting the locked dependencies, which would resolve some of the problems here. But this then means the pins in setup.py are completely irrelevant for the docker images (but still applicable for the non-containerised integration tests). So ideally we need to also ensure the constraints in setup.py are automatically incorporated into the Pipfiles.

@jc-harrison jc-harrison added bug Something isn't working FlowClient Issues related to FlowClient FlowAuth Issues related to FlowAuth FlowMachine Issues related to FlowMachine FlowAPI Issues related to the FlowKit API dependencies Pull requests that update a dependency file flowkit-jwt-generator labels Mar 29, 2023
@Thingus
Copy link
Contributor

Thingus commented Mar 29, 2023

What do we want our single source of truth to be; setup.py or the Pipfile? Given that setup.py is a Python script underneath it all, would the right move here be to replace the requirements section in setup.py with something that goes and grabs the requirements out of the Pipfile on install?

The result is that dependabot can bump version pins in a Pipfile, and integration tests will continue to use the older versions pinned in setup.py, so Pipfile pins can be "successfully" bumped to versions that would break the integration tests.

Just so I'm clear - this is undesirable behaviour, right? We want the intergration tests to be in step with the rest of the distribution.

@jc-harrison
Copy link
Member Author

What do we want our single source of truth to be; setup.py or the Pipfile? Given that setup.py is a Python script underneath it all, would the right move here be to replace the requirements section in setup.py with something that goes and grabs the requirements out of the Pipfile on install?

I think setup.py should be the source of truth - the other way around feels wrong to me, and not in keeping with the direction of python packaging overall moving towards static config (i.e. pyproject.toml instead of setup.py). The pipenv docs also recommend telling pipenv to lock the setup.py-declared dependencies, not the other way around.

It may be sufficient to stick an editable install of the package in the Pipfile (and remove the pinned requirements duplicated from setup.py) - this is the recommended way to do it, and I can't see an obvious reason for this to be a bad idea. Given the difficulties dependabot has with editable requirements from a local path, though, this change may further break the usefulness of dependabot.

Just so I'm clear - this is undesirable behaviour, right? We want the intergration tests to be in step with the rest of the distribution.

Correct - we want everything to be tested (and deployed) using the pipenv-locked dependencies.

@greenape
Copy link
Member

Tbh I think the bot is counter productive at this point, my speculation would be that the way to go is a gh action for doing the bumping.

@jc-harrison
Copy link
Member Author

It's worth noting that pip-tools can lock a setup.py directly, rather than needing a separate input file. Although as I found when switching flowetl over from pipenv to pip-tools, we'd then have to add a fair bit more boilerplate for managing virtual envs.

@Thingus
Copy link
Contributor

Thingus commented Mar 29, 2023

So a Pipfile would just be

[packages]
flowmachine = {editable = true, path = "."}

and install_requires gets eveything that was in [packages]?

@jc-harrison
Copy link
Member Author

Yep. In most cases, I think install_requires should already have everything that's in [packages].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file FlowAPI Issues related to the FlowKit API FlowAuth Issues related to FlowAuth FlowClient Issues related to FlowClient flowkit-jwt-generator FlowMachine Issues related to FlowMachine
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants