Skip to content

feat: Support requirements layer caching #644

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

Merged
merged 9 commits into from
Dec 20, 2021

Conversation

mLupine
Copy link
Contributor

@mLupine mLupine commented Nov 20, 2021

Closes #148.

Unfortunately, even though I managed to make the code generate the same layer zip file every time, JSZip always generates an archive that differs from the previous one by just a few bits. This is enough, however, to change the checksum of the generated archive and make Serverless reupload the layer archive despite no changes being made to Python dependencies.

So, instead of making the archive file look the same on every deploy, I took a different approach and made the plugin actually use the same archive on every deploy (unless requirements changed, of course). The plugin now generates the layer zip in the cache directory and creates a symlink to that file inside .serverless, resulting in shorter deployments times with no side effects.

@pgrzesik
Copy link
Contributor

pgrzesik commented Nov 25, 2021

Hello @mLupine - thanks a lot for the PR. Do you happen to know why JSZip always creates differing archives? Also, could you please rebase your PR on top of current master? Thanks in advance 🙇

@mLupine
Copy link
Contributor Author

mLupine commented Nov 25, 2021

Hi @pgrzesik, unfortunately, I didn't dig into JSZip deep enough to find out the cause of the problem. I only found this issue and it was enough to convince me that it was a problem with the library itself.

@pgrzesik
Copy link
Contributor

Thanks a lot for providing more context @mLupine - I wonder if we could replace zipping library to something more deterministic, but that might increase the scope - I'm still getting up to speed on some of the parts of the plugin, I'll dive into the changes later this week

@mLupine
Copy link
Contributor Author

mLupine commented Nov 30, 2021

While I fully agree that ZIP generation should be deterministic, I think that caching the generated file should be implemented in the library anyway.

Having the archive with the same SHA hash would speed up deployments from multiple machines (i.e. made by different team members), but having the ZIP already cached speeds up the deployment even more. With the changes from this PR, the plugin only checks if the file is present in the cache directory and skips the compression process completely. It's just a few seconds, but it actually makes the local deployments more smooth.

Maciej Wilczyński added 7 commits December 1, 2021 11:48
* master: (24 commits)
  chore: Remove Node16 tests
  chore: Remove dependabot
  chore: Reformat with eslint & prettier
  ci: Add commitlint job to CI
  ci: Introduce new CI publish workflow
  ci: Introduce integrate CI workflow
  ci: Update validate CI workflow
  refactor: Use `ServerlessError` in `pip`
  refactor: Use `ServerlessError` in `pipenv`
  refactor: Use `ServerlessError` in `poetry`
  refactor: Use `ServerlessError` in `docker`
  refactor: Cleanup and use `finally` for code simplification
  refactor: Ensure proper verbose progress logs
  refactor: Adapt `docker` for modern logs
  refactor: Adapt `inject` to modern logs
  refactor: Adapt `layer` to modern logs
  refactor: Adapt `pip` to modern logs
  refactor: Adapt `zip` to modern logs
  refactor: Adapt `shared` to modern logs
  refactor: Adapt `clean` to modern logs
  ...
* master:
  feat: Add architecture to requirements cache directory name (serverless#645)
@mLupine
Copy link
Contributor Author

mLupine commented Dec 17, 2021

Hi @pgrzesik, is there anything left to be done in this PR? I think it'd be great to have it merged as it greatly improves consecutive deployment times :)

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thanks @mLupine and sorry for the delay, I was on vacations and just came back last Friday. The PR looks good to me, the only thing that needs to be kept in mind I guess is the fact that we're imposing that if there are no requirements.txt change then there's no change with actually resolved deps which not always will be the case (e.g. when versions are not strictly pinned) but I think it's okay in this situation.

@pgrzesik pgrzesik changed the title Implement requirements layer caching feat: Support requirements layer caching Dec 20, 2021
@pgrzesik pgrzesik merged commit 406f6ba into serverless:master Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip package/deployment steps if nothing changed
2 participants