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: updating poetry.lock before local wheels are used in solution template #495

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

JoseRamonRodriguez
Copy link
Contributor

It is considered as a good practice that whenever you change the pyproject.toml file, you should run poetry lock --no-update afterwards to sync the poetry.lock files with those changes. This is what we do when some packages are provided as wheel files through the setup_environment.py. The --no-update flag tries to preserve existing versions of dependencies.

In this PR I am executing poetry lock --no-update as part of setup_environment.py before installing dependencies.

@JoseRamonRodriguez JoseRamonRodriguez added the bug Something isn't working label Jun 28, 2024
@JoseRamonRodriguez JoseRamonRodriguez requested a review from a team as a code owner June 28, 2024 07:39
@JoseRamonRodriguez JoseRamonRodriguez enabled auto-merge (squash) July 1, 2024 07:21
@JoseRamonRodriguez JoseRamonRodriguez merged commit dac81eb into main Jul 1, 2024
62 checks passed
@JoseRamonRodriguez JoseRamonRodriguez deleted the jrodrigu-setup_environment_solutions branch July 1, 2024 07:38
@nick-marnik
Copy link
Contributor

@JoseRamonRodriguez Can you please elaborate on the context that triggered the need for this change?

I am not totally opposed to it, but I'm not sure it is the right approach to take. To prevent situations where the TOML file and the lock file get out-of-sync, we use:

  • pre-commit hooks for local development
  • CI/CD checks (that should be enforced at the PR level)

Specific to the solution template in this repo, we also rely on the E2E solution tests to validate the lock file since the TOML file contains placeholders and is technically invalid TOML. These tests tend to break and I'm not sure they are monitored, so we may need to investigate their current state.

We also have the -F option available on the setup script to allow developers to delete the poetry.lock file.

My concerns:

  • If there are situations where these files are out-of-sync, we should try to understand how that is happening and enforce it at the CI/CD level instead
  • I think that there should be manual intervention for a developer to update the poetry.lock file. With this change, a developer could be setting up a clean development environment and end up with local changes that they do not understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants