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

Skip package/deployment steps if nothing changed #148

Closed
AdrianSanchezLopez opened this issue Feb 28, 2018 · 30 comments · Fixed by #644
Closed

Skip package/deployment steps if nothing changed #148

AdrianSanchezLopez opened this issue Feb 28, 2018 · 30 comments · Fixed by #644

Comments

@AdrianSanchezLopez
Copy link

Would it be possible to completely skip the package and deploy steps if nothing actually changed? equal to what the vanilla Serveless does, if no code, dependencies and serverless.yml files changed it skips all the package, upload to provider and deployment steps.

At the moment, if you run sls deploy with this plugin, and re-run it again without any changes at all, it will re-package and re-deploy everything again (as a new version).

This improvement would help a lot in cases like mine, where we have a CD/CI based on a Monorepo, that will automatically deploy and skip when there were no changes.

@AndrewFarley
Copy link
Collaborator

So... this feature sounds like it needs to be a serverless feature itself, not a serverless-python-requirements feature.

Given the scope of this plugin, however, I implemented a proof-of-concept that does something similar to this feature and might do enough of what you want. It doesn't re-gather the requirements if requirements.txt hasn't changed. Please see #162 . This helps us cut down on build times greatly, but it of course still re-generates the zip file.

Just my 2c, but I'd say that the scope of this issue is too large, and should be "won't fix" because it needs to be implemented at the serverless level, not at the python requirements plugin level.

@AdrianSanchezLopez
Copy link
Author

Hi @AndrewFarley thanks for replying, but that's my point, it is already a Serveless feature.
What I'm saying is that this plugin somehow overrides/breaks that feature.
As I said on the Issue topic: currently in Serveless, if no code, dependencies and serverless.yml files changed it skips all the package, upload and deployment steps.

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented Mar 14, 2018

@Adriayala I honestly had no idea that serverless even did this until you mentioned it (I've always had to be using this plugin). I just did a brief test of that feature, and you're right... it says Serverless: Service files not changed. Skipping deployment... when not using this plugin, lol. Amazing!

I just peeked at the serverless code that detects and does this feature. It basically still generates the .zip file locally, and then compares the hash of the local zip file against the hash of the file that was deployed. So, what that says to me, is the process of re-generating the pip dependencies doesn't have consistent output. I believe a future version of my proposed merge, re-using the same exact dependencies, will actually make this feature work right.

I'm working out the kinks, but from what I can see, I believe it will.

This really sounds like a wonderful feature, and I'd love to see it as well. I'm going to do some initial tests with some of my ideas about my PR (moving the data into a temp folder) to see if I can get this state to trigger at least once. If I can, then we're on the right path. Thanks for bringing this to light. Keep you posted...

@dschep
Copy link
Collaborator

dschep commented Mar 14, 2018

@dschep
Copy link
Collaborator

dschep commented Mar 14, 2018

(to try it, install the v4 beta with npm i serverless-python-requirements@beta)

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented Mar 14, 2018

@dschep You're on the right track. I was just digging into this and saw Date(0) is the trick that serverless uses to achieve this which I saw you using in that PR, however this patch doesn't quite fix it. After extracting the .zip with the above beta I still see current modification times of the FOLDERS, but the file times are okay (although far in the future, instead of in the past, but same end-result). Maybe just one more flag to also set the folders modification times?

$ ls -la
drwxr-xr-x   9 farley  staff   288 Mar 14 13:48 PyYAML-3.12.dist-info
-rw-r--r--   1 farley  staff   500 Jan  1  1980 handler.py
drwxr-xr-x  65 farley  staff  2080 Mar 14 13:48 node_modules
-rw-r--r--   1 farley  staff     7 Jan  1  1980 requirements.txt
drwxr-xr-x  36 farley  staff  1152 Mar 14 13:48 yaml
$ ls -la PyYAML-3.12.dist-info/
-rw-r--r--  1 farley  staff   534 Jan  1  2098 DESCRIPTION.rst
-rw-r--r--  1 farley  staff     4 Jan  1  2098 INSTALLER

@AndrewFarley
Copy link
Collaborator

Let me know if you have another beta to try, I've a few projects I can test them on.

@dschep
Copy link
Collaborator

dschep commented Mar 14, 2018

Will do @AndrewFarley

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented Apr 4, 2018

While workin' on my merge... this problem piqued my interest. I noticed we're using zip-local module which is basically abandonware. I recommend modifying this plugin's zipping to use the same module that serverless does, which is a well supported and widely used module node-archiver. Anyone interested feel free to pick this up. Please see:

https://github.com/serverless/serverless/blob/master/lib/plugins/package/lib/zipService.js#L83

Which is a reference from the serverless framework of how they do it. I believe that archiver module properly sets the modification/creation times based on your input of all folder paths for a file you add to a zip.

@dschep
Copy link
Collaborator

dschep commented Apr 4, 2018

I like the idea of using the same zip code. I initially did exactly that: https://github.com/UnitedIncome/serverless-python-requirements/blob/072915b546f53a914cae9c0b4ac01c0d6dffcd9b/lib/zipService.js

@AdrianSanchezLopez
Copy link
Author

Hey, I'm glad to see you guys are making progress in here. So what's the plan, using that zip code instead of the current one? and is there an ETA?
Just asking for adjusting our team development, to see if it is possible for us to keep using this plugin our find an alternative in case this issue could take too much time.
Thanks in advance!

@dschep
Copy link
Collaborator

dschep commented Apr 9, 2018

It's not on my(@unitedincome's) roadmap, but I'll merge it as soon as it's done. You can follow @AndrewFarley's progress on #165

@AndrewFarley
Copy link
Collaborator

I would love to work on this next I know I could use it, but I’m neck deep with my other merge hoping to finish that in a week or two. But if I can I’d say maybe a month and I’ll have some code for this then will just need some reviewing.

@jacintoArias
Copy link

Hi, is someone still looking into this?

I don't know if the addition of lambda layers changes how this is behaving. Running serverless 1.38.0 under macos I still have to upload both lambda and layers .zips every time we deploy a service, even if anything changes.

@levFarkas
Copy link

Hi, is someone working on this?

The issue is still up, and uploading even nothing has changed takes too much time.

@AndrewFarley
Copy link
Collaborator

No one is working on this, but if anyone wants to pick this up, please do.

@grvhi
Copy link

grvhi commented Jun 16, 2019

At the risk of crossing requirements here, it would seem there's an opportunity here to add a "skip layer building" option as well...? The idea being that if this plugin has been used to deploy python requirements as an independent layer, it should also support options for the user to choose to independently build (and deploy) either the requirements layer or the function code?

@xv-alex-t
Copy link

Really really would appreciate this, its a major pain point for us

@tvb
Copy link

tvb commented Apr 29, 2020

Where are we with this feature request? My functions are being rebuild each time I deploy even if they or the requirements haven't changed at all.

@brettdh
Copy link

brettdh commented May 27, 2021

I'm interested in working on this, but I'm a bit confused on the state of things, since both the items mentioned above as potentially resolving this issue are now done:

  • The project seems to be using jszip rather than zip-local now
  • The static caching PR was merged and released

Did those perhaps just turn out not to have the impact on this issue that you thought?

@AndrewFarley What are the next steps here? ("Figuring out the next steps" is a fine answer; just wanted to check before I start off in potentially the wrong direction 🙂)

@AndrewFarley
Copy link
Collaborator

@brettdh There's some history from before when I joined this codebase, but the serverless framework does use zip-local and the solution to this should be moving this plugin over to that framework which would allow the timestamps to be set properly and then would be able to detect the lack-of-changes, as the serverless framework does.

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented May 27, 2021

As far as static-caching, that MR allows caching of having to re-run pip to compile modules, but when it is injected into the serverless package because of timestamps of the folder as a result of a flaw of the jszip plugin it will never generate the same .zip file from every build.

@brettdh
Copy link

brettdh commented May 27, 2021

the serverless framework does use zip-local

Are you sure? I don't see it in the package.json, but I do see jszip there. The earlier message said zip-local had the timestamp flaw; are you saying jszip has a similar flaw?

the solution to this should be moving this plugin over to that framework

I'm confused; do you mean moving this plugin to use a different zip module? (I'm not sure what "moving this olugin over to that framework" means otherwise.) And which zip module will set the timestamps correctly? At first I thought it was node-archiver, but from your notes today I'm not sure.

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented May 27, 2021

@brettdh Actually, you're right someone must have merged something without me noticing that we've switched over to jszip. So, then this issue should be resolved already, or it should be fairly trivial to resolve. Aka, there might be some slight tweaks necessary to our usage of jszip to ensure we do things "the same". Making our inputs generate exactly the same binary (zip) output, which serverless detects and will then skip. The problem previously when using zip-local was that all folders created had an date which was a timestamp of now and this was non-configurable.

I haven't looked into this issue in a while now, but hopefully with this insight you can debug this. If you'd like to look into this please do. Just run serverless package twice in a row with not changing anything. Save the zip file generated in the .serverless folder after your first run, and compare its contents with the second zip. When you find what doesn't match, come back here and report it, and/or PR a fix for it! :)

(Note: IIRC, the serverless framework makes all folders/files it creates in the .zip file to have "epoch" as the date/time for consistency between builds)

@brettdh
Copy link

brettdh commented May 27, 2021

Yep, happy to look into it further, especially if what remains might be trivial to resolve. 🙂

I'm also curious about whether you'd expect the static caching and detection-of-no-changes also would apply to the .zip generated for a lambda layer (assuming we resolve whatever jszip config issue remains), or if further changes would be needed to address that.

@AndrewFarley
Copy link
Collaborator

AndrewFarley commented May 27, 2021

@brettdh The static caching should be unaffected by the desire for and/or changes as a result of this feature. If you need to tweak anything, it would be in the "copy" step into the .zip file (eg: to change its datetime to epoch). This code lives "outside" of the static caching logic.

Note: This feature/concept has never been possible with this plugin. By fixing this, you'd probably make a ton of people that use this plugin really happy and giddy.

@brettdh
Copy link

brettdh commented Jun 16, 2021

Update: found the spot that needed a fixed timestamp; set the fixed timestamp; got a consistent .zip file output on each sls package. However, I also broke something that caused lambda to reject my zipfile, so I'm still working on it.

@mLupine
Copy link
Contributor

mLupine commented Sep 8, 2021

Hi @brettdh, were you able to pinpoint what's the issue with your zip files? If not, or you need a fresh pair of eyes to look into the issue, I'll be happy to assist. I'd really like to get this feature merged 😉

@relsunkaev
Copy link

It seems #644 doesn't fully solve this. The compression step is skipped but a new layer still gets deployed even if the requirements didn’t change.

@dongho-jung
Copy link

I'm suffering with the exact same issue. After some investigating, I found the same thing with @relsunkaev

It seems #644 doesn't fully solve this. The compression step is skipped but a new layer still gets deployed even if the requirements didn’t change.

In some cases, different last modification time makes some troubles to cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.