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 nightly build #2499

Merged
merged 7 commits into from
Jan 12, 2021
Merged

Fix nightly build #2499

merged 7 commits into from
Jan 12, 2021

Conversation

cygnusv
Copy link
Member

@cygnusv cygnusv commented Jan 6, 2021

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Our nightly build has been failing for months, but due to minor problems. This PR fixes them.

Issues fixed/closed:
Fixes #2380

Why it's needed:
No long-term goals are directly affected, but fixing the nightly build has been pending for too long 😅

Notes for reviewers:
There's nothing particularly fancy about this PR. Perhaps understanding the root of the problems is useful:

  • The jobs related to building with pip for other python versions were failing because it was missing a permission on some dependency path
  • The validate_reqs_files job was doomed to failure since it wasn't consistent with what the relock_dependencies.sh script does. To this respect, this PR adds an optional flag to this script so we can tell it to build the dependency files from the existing Pipfile.lock, instead the default behaviour which does everything from the Pipfile; the former makes the process deterministic, while the latter makes the result different if some 3rd party dependency gets a new release, which was making the job to fail.
  • Checking this PR against the nightly build is tricky, because it's a different workflow on CI. For reviewers' convenience (and mine!), I forced the affected jobs to run in the normal build. You can see it here:
    https://app.circleci.com/pipelines/github/nucypher/nucypher/6894/workflows/d56f4a33-e707-45c5-a57f-2ad3075f8afc

cygnusv added a commit to cygnusv/nucypher that referenced this pull request Jan 6, 2021
cygnusv added a commit to cygnusv/nucypher that referenced this pull request Jan 6, 2021
cygnusv added a commit to cygnusv/nucypher that referenced this pull request Jan 12, 2021
…m the existing Pipfile.lock

Otherwise, the default is to create a lock file, and then create the requirements files. The motivation of this change is that the validate_reqs_files job on CI wasn't deterministic, as it was failing everytime some dependency was updated (and therefore, the produced dependency file was updated and different to the existing in the codebase)
Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤠

@@ -333,6 +341,12 @@ python_38_base: &python_38_base
docker:
- image: circleci/python:3.8

python_39_base: &python_39_base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out with the old in with the new

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow. this is the lord's work!
🙏

OPTIND=1
while getopts 'k' opt; do
case $opt in
k) KEEP_LOCK=true ;;
Copy link
Member

@derekpierre derekpierre Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KPrasch , @cygnusv the Pipfile.lock does not contain the the dependencies for the docs-requirements file - is this an issue if it it kept and reused? There is currently no Pipfile.lock for the docs requirements kept in the repos, only a Pipfile. So running the script and using an old Pipfile.lock would keep the dev/standard requirement files the same, but the docs-requirements.txt would still be freshly generated...?

Should we maintain a Pipfile.lock file as well for the docs-requirements?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on how "proper" we want to be about Pipenv. In practice the Pipfile.lock is only used for pipenv install (and other pipenv management commands). Since we're typically using the Pipfile to generate a fresh lock, and ultimately a requirements file, I'm indifferent here.

is this an issue if it it kept and reused?

No, but it's not much of a help either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For regular relocking, yes no problem there. But if this script is used for a nightly build, and the dev/standard requirements are kept the same ('k' option used), and the docs-requirements are updated to newer version, does that lead to any pip installation issues, like what we observed when the docs-requirements were not relocked at the same time as the dev/standard requirements i.e. issues with pip install -e .[dev] prior to PR #2510 ? Or is the nightly build sufficiently different in some way that this problem won't rear its ugly head?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekpierre running the script with the old pipfile is mainly motivated as a way to validate that the requirement files are consistent with the pipfile. The docs requirements are not validated, so it's not much of a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸

@vepkenez vepkenez merged commit e7be542 into nucypher:main Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly build failure (dependency locking)
4 participants