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

Re-Enable Parallel Poetry Install #1674

Merged
merged 7 commits into from
Apr 22, 2022
Merged

Re-Enable Parallel Poetry Install #1674

merged 7 commits into from
Apr 22, 2022

Conversation

bryanculver
Copy link
Member

@bryanculver bryanculver commented Apr 21, 2022

Closes: #DNE

What's Changed

We have the statement:

# Poetry 1.1.0 added parallel installation as an option;
# unfortunately it seems to have some issues with installing/updating "requests" and "certifi"
# while simultaneously atttempting to *use* those packages to install other packages.
# For now we disable it.

in our Dockerfile and this seems to be one of the slowest parts of our CI on integration and release (ARM now making it even slower, 40+ min).

This has worked locally for me for a while so want to do some test runs removing it in CI.

TODO

  • Explanation of Change(s)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@bryanculver bryanculver marked this pull request as draft April 21, 2022 18:22
@bryanculver
Copy link
Member Author

Found the original introduction of this and it seems to have been due to intermittent CI issues in TravisCI: a86ea83

Maybe that's changed?

I'll cycle this CI a few times to see if I can get it to hiccup again.

@bryanculver
Copy link
Member Author

Appears to be about a 30% performance improvement (AMD finishes fast, ARM still takes a while due to emulation):

Current: https://github.com/nautobot/nautobot/runs/6116614633?check_suite_focus=true
Parallel: https://github.com/nautobot/nautobot/runs/6117868093?check_suite_focus=true

30min vs 45min ~= 33% performance improvement.

Sometimes you get weird runs though:

Current: https://github.com/nautobot/nautobot/runs/6116614413?check_suite_focus=true
Parallel: https://github.com/nautobot/nautobot/runs/6117867847?check_suite_focus=true

57min vs 1h 6m

@bryanculver bryanculver marked this pull request as ready for review April 21, 2022 20:40
@bryanculver bryanculver changed the title WIP: Trial run of disabling parallel poetry install Re-Enable Parallel Poetry Install Apr 21, 2022
@glennmatthews
Copy link
Contributor

Once bitten, twice shy here personally. I'd rather have a slower but reliable build versus a faster one that fails intermittently. I'm not aware of any specific fixes in Poetry 1.1.x that would resolve the intermittent failures we were encountering at the time; I also don't see any reason to believe that the failures were specific to Travis CI and unlikely to also occur under GitHub Actions.

Perhaps a middle ground might be to add a Dockerfile ARG that could be used to control whether a given build is parallel or not? Local builds and ci_integration builds could use PARALLEL=true for speed but less reliability, while prerelease/release builds could continue at least for now to use what we know works reliably?

@bryanculver
Copy link
Member Author

Once bitten, twice shy here personally. I'd rather have a slower but reliable build versus a faster one that fails intermittently. I'm not aware of any specific fixes in Poetry 1.1.x that would resolve the intermittent failures we were encountering at the time; I also don't see any reason to believe that the failures were specific to Travis CI and unlikely to also occur under GitHub Actions.

Perhaps a middle ground might be to add a Dockerfile ARG that could be used to control whether a given build is parallel or not? Local builds and ci_integration builds could use PARALLEL=true for speed but less reliability, while prerelease/release builds could continue at least for now to use what we know works reliably?

Changed.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Thank you!

@bryanculver bryanculver merged commit d62f2db into develop Apr 22, 2022
bryanculver added a commit that referenced this pull request Apr 22, 2022
@bryanculver bryanculver deleted the bsc-parallel-poetry branch May 2, 2022 04:15
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.

3 participants