Download and static caching as a feature

[AndrewFarley]

Fixes #157 (filed by me)

What this does

• Makes the download caching of pip a "first-class citizen" as an option directly in this plugin's options. This will "fix" a few (attempts) at using the pip cache, specifically in Docker, and will simplify this feature (as the user simply has to enable it, not specify a folder). In a future MR, I'd highly suggest enabling this by default.
• Second, it adds a new type of caching called "static caching" which allows you to cache the outputs of this plugin. This greatly speeds up every single build as long as you have the feature enabled and do not change your requirements.txt file. In a future MR, I'd highly suggest enabling this by default also.
• The pip download and static cache are shared between any projects of the same user through an appdir cache folder when packaging your service. This especially helps on projects that heavily use Docker (Win/Mac) for deployments or development, or for pip modules that need to compile every time, and especially for projects with long requirements.txt files. This will also greatly help the longer and more complex your requirements.txt is, and/or if you use the same requirements.txt on multiple projects (common in team environments).

Implementation details

• When either cache is enabled, this plugin now caches those requirements (download or static) to an "appdir" cache folder (per the appdirectory node module).
• When this feature is NOT enabled, nothing changes
• Injection happens directly from the new cached requirements directory via a symlink created in the right place in .serverless or .serverless/functionname if deploying individually.
• As mentioned above, there is a symlink into the .serverless folder when the static cache is enabled pointing to it, so you still "know" where your cache is (for both individually and non-individually packaged functions).
• The requirements.txt "generator" was improved to remove comments, empty lines, and sort the list of items before trying to use it (or check its md5 sum). This allows for more actual md5 matches between projects, in-case of comments and such in the requirements file.
• A new command was added to the command-line to flush the download/static cache, called cleanCache invokable with: serverless requirements cleanCache. This clears all items including the download and static cache.
• A handful of new tests were created for various edge conditions I've found while doing this refactoring, some were based on bugs other people found while using this plugin with some combination of options, some are not directly related to this merge's intent, but it's just part of my stream of work/consciousness. Sorry tests take a lot longer to run now since there are lots more now.
• A UID bug fix related to docker + pip was implemented (seen on a few other bugs) from @cgrimal
• The following new configurable custom options were added to this plugin...

Variable Name	Value	Description
useStaticCache	false/true	Default: false. This will enable or disable the static cache. After some testing I would like to make this default: true, as this will greatly help everyone, and there's no reason to not enable this. Possibly making this default: true will help weed out issues faster. I'll gladly step-up to quickly fix any bugs people have with it since I'm now well accustomed with the code.
useDownloadCache	false/true	Default: false. This will enable or disable the pip download cache. This was previously the "example" code using a pipEnvExtraCmd to specify a local folder to cache downloads to. This does not require a cache location to be set, if not specified it will use an appdirs.usercache() location.
cacheLocation	<path>	Default: appdirectory.userCache(appName: serverless-python-requirements) This will allow the user to specify where the caches (both static and download) should be stored. This will be useful for people who want to do advanced things like storing cache globally shared between users, or for CI/CD build servers on shared-storage to allow multiple build machines to leverage a cache to speed builds up. An example would be to mount a shared NFS store on all your CI/CD runners to /mnt/shared and set this value to /mnt/shared/sls-py-cache.
staticCacheMaxVersions	<integer>	Default: 0. This will restrict the a maximum number of caches in the cache folder. Setting to 0 makes no maximum number of versions. This will be useful for build/CI/CD machines that have limited disk space and don't want to (potentially) infinitely cache hundreds/thousands of versions of items in cache. Although, I would be disturbed if a project had hundreds of changes to their requirements.txt file.

TODO

• Feature Implementation
• BUG: Deploying single-functions fails (Packaging works, but fails because of Fatal error when trying to deploy single-functions #161 )
• Code Styling / Linting
• Test to be sure Pipfile / generated requirements.txt still works
• Tested a bunch on Mac / Linux with and without Docker
• Adding Tests for Download Cache
• Make sure zip feature still works
• Ensure all existing tests pass
• Adding Tests for static cache
• Updating README.md to inform users how to use it
• Make sure dockerSsh works
• Implement error when trying to use --cache-dir with dockerizePip (won't work)
• Implement suggestion when trying to use --cache-dir without dockerizePip
• Test on Windows
• Iterate through any feedback
• Rebase with master constantly, awaiting merge... :)

Replaces #162

[AndrewFarley]

@dschep @Valian Please check this out... functionally complete I believe, at least from what I can tell. :)

[AndrewFarley]

For anyone wishing to try this and provide feedback change into your serverless project directory and remove the official serverless-python-requirements and install this one with...

rm -Rf node_modules/serverless-python-requirements
npm i github:andrewfarley/serverless-python-requirements#master

Then modify your serverless.yml to include the useStaticCache: true...

...
custom: 
  pythonRequirements:
    dockerizePip: true
    useStaticCache: true
...

Then try a serverless package operation (or serverless deploy operation) twice, the first time it'll save all outputs to cache, the second time it will skip using docker (or pip without docker) and just use those cached compiled outputs directly and inject them into the zip immediately

[dschep]

I'll try and check it out soon. And fix up the linting errors, please: https://circleci.com/gh/UnitedIncome/serverless-python-requirements/345?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

[AndrewFarley]

@dschep Feel free to just give this PR a brief glance, don't look too intensely yet. Just want to know if there's something I'm doing that is fundamentally wrong, let me know so I can re-align. Just fixed linting, but still have some test failures.

I spent a little time this evening looking into some test failures... so far I have one major problem with a test. The py3.6 uses cache with dockerizePip option test. I'd like to propose the following...

• Add a new config variable, called pipDownloadCache: true/false, which when set to true, automatically creates a folder for a download cache in an appdirs.userCache() location. When true, ALL serverless projects on the same machine will share this download cache location automatically (when executed by the same user). This greatly simplifies the "ease-of-use" to begin caching things with this plugin.
• Add a new config variable, called pipDownloadCachePath: , which when specified automatically sets the pipDownloadCache to true, and it uses the specified path instead of using an automatic appdirs() cache path.
• Modify the tests and README related to pip caching to use this new variable, and not to put this in the pipExtraCmd
• Add a fatal error (or just cli output warning...?) when a string match detects someone trying to use the download cache as a pipExtraCmd option (aka, the old way)
• Update the pip logic to use this path if specified as an extra option to pip, when using Docker, it will volume mount this path

Proposed config with a static and download cache (without custom path) would be

...
custom: 
  pythonRequirements:
    dockerizePip: true
    useDownloadCache: true
    useStaticCache: true
...

This is necessary and desirable because this makes this feature more "standard" in this plugin and not added as an extra pip option with two commands. This came up because this pull request changes fundamentally how the Dockerization works. In the past, this plugin has been quite famous for "leaving trash" all over your project root if/when it fails for some fatal reason because we mounted the entire servicePath into Docker. To me, that was a fundamentally flawed approach and I saw it all the time, all that Docker needs is a requirements.txt file and a temporary folder volume mount to spit its outputs into. So, in this PR, Docker was adjusted in this way works and mounts only a dedicated cache/temp sub-directory into Docker, not your entire-project-directory. This is for security, integrity, scope, and static caching purposes.

I'll work on implementing this next, and adding/modifying tests to cover this.

[AndrewFarley]

A little closer, download cache is now a built-in feature, which works without having to specify extraCmd or a path for it. The cache path is now shared between both the static and the download path, for simplification. And now almost all (but one) tests pass, and some new tests exist. I'm wrestling with a small part of it, just need some more time to debug it.

[Valian]

@AndrewFarley I'm using your version of serverless-python-requirements for a couple days, and it works as intended using useStaticCache: true. Not found time yet to test all options, though. My deployment time dropped from ~4 minutes to ~1 minute, so I'm really impressed, great job :) I'm using Ubuntu 16.04.

[AndrewFarley]

Thanks for more sets of eyes and/or trying it @Valian. Good to hear you've seen the same results. I'm slammed with work this week but I hope to finish up the last tests and do a few more tweaks so we can get this in mainline soon.

[dschep]

That's great news @Valian! Thanks for the testing! 😄

[cgrimal]

Hi @AndrewFarley!

I tested your PR with the dockerSsh (and even the dockerFile option) and even though it did not work at first, I managed to make it work with a minor patch.
I don't know if there is a simple way to communicate it to you, so I'll try like that as it is simple. In the block starting at line 132, replace:

       // Use same user so requirements folder is not root and so --cache-dir works
       cmdOptions.push('-u', `${process.getuid()}`);

by

      // Set the ownership of the folder to current user
      pipCmd = quote(pipCmd);
      const chownCmd = quote([
        'chown', '-R', `${process.getuid()}:${process.getgid()}`, '/var/task',
      ]);
      pipCmd = ['/bin/bash', '-c', '"' + pipCmd + ' && ' + chownCmd + '"'];

You'll need to add this import as well:

const {quote} = require('shell-quote');

See my comment here as well: #182 (comment) as I don't know if @dschep will want to keep it like that.

Thanks!

[AndrewFarley]

@cgrimal Thanks for the mini patch, I'll try to apply it, but I don't know how to test it. Have a few minutes to chat? I'd love to understand, especially from the author, what that feature is supposed to do exactly. Ping me, farley at neonsurge dot com. I'd like to make some time asap to finish this and one of the blockers is indeed the dockerssh. Would love your help. I could add you as a admin on my fork and we could work together on it or something?

[AndrewFarley]

I'll spend some time now rebasing and resolving the conflicts to bring my MR back up to date, so we can move forward.

[kichik]

@cgrimal can you please provide more details on how it fails with -u? I replaced chmod with it originally so --cache-dir starts working in #145.

[AndrewFarley]

Just did the merge, resolving conflicts with the latest from master, it fixed a handful of bugs that I had already sort-of coded around... I haven't gone and ran the unit tests or linting yet... working on it...

[AndrewFarley]

Hey guys, so, back at it again and think it's ready to go, rebased and refactored. All tests are passing locally but don't seem to pass on CircleCI, I'll try to fix this soon if I can figure it out. But if I could get @cgrimal to test your UID fix in this MR and to maybe test the DockerSSH overall? And @Valian / @dschep could you re-review? I refactored this quite a bit to change less code, be simpler, and work more like it used to in every way when the new features are disabled. Besides that just need to manually spot check on Windows. I'll wait till there's no other feedback/stoppers and do some final testing there.

[cgrimal]

Great work @AndrewFarley!

I tested it on my current project, and it works almost like a charm! The dockerSsh works as expected.

Even though I am the author of this feature, @dschep helped me quite a lot by suggesting great ideas that ended up being implemented in the PR!

Eventually, what it does is quite straightforward. It links the files used by the SSH agent from your .ssh directory to the .ssh directory of the user inside the Docker container. By default, the Docker image is lambci/lambda:build-python3.6 and it assume the user is root (as on AWS Lambda), that's why the used .ssh directory is located in the /root folder.

If you run the Docker process as your current user, the SSH agent won't find your SSH keys because it will look for the .ssh directory inside the user's home directory, and it does not exist.

I remember that at first, I tried to keep the option passing the user id, but none of the workarounds I came up with manage to do the trick.

@kichik: Regarding the issue with the --cache-dir, I'll be glad to have a minimal example to understand how it breaks. That being said, the caching feature is already using directly this pip option, so I'm not sure it will be necessary to keep using it as an additional pip argument.

Unfortunately, I found out the vendor option is broken for individual package, when this option is set at function level. Indeed, you only check if options.vendor is set, when it can also be f.vendor.

A minor remark as well is that we should escape all the path to volumes, including the ones related to dockerSsh, as you did for the path to the cache directory.

If you want to add me as admin, I'll be glad to contribute (I don't know how much time I'll be able to put though), or I could do a PR on your branch?

Many thanks guys, I'm really looking forward to using this feature!
Thank you for reading me until the end, I hope I did not forget anything.

[AndrewFarley]

@cgrimal You raised a few interesting points...

• First, I will probably want to throw an error for people that are/will try to use --cache-dir still as a manual pip option. I updated the readme and removed this example, but, I should also fail if you're trying to use this plugin with that. This will help people re-align to use the now-built-in feature. I already thought of this but totally forgot to implement this. Any opinions? Should I just throw a WARNING and try to proceed...? Or should I error out? Anyone have an opinion?
• Second, I'll do some more testing about vendor stuff with individual packages. I don't think we had a test for this, so I'll add a new test for this, and then fix it. I don't know if this was a problem in master, or a new problem? Either way, I'll add a test to cover it.
• Finally, good point about escaping paths, I'll try to get around to doing this, but if you have more knowledge/time in this regard, you're welcome to add it. I'll go add you as a contributor to my fork now.

[cgrimal]

Thanks for the quick feedback @AndrewFarley.
On your 3 points:

• Good idea, don't have any strong opinion about that, but @dschep should decide in the end I guess. If the user try to use --cache-dir, will it break completely or not?
• This is a new problem. On master, the vendor handling is done outside the installRequirements function, in the installAllRequirements: https://github.com/UnitedIncome/serverless-python-requirements/blob/master/lib/pip.js#L222
• This should be very straightforward, thanks!

[cgrimal]

Oh snap!
When the docker command is successful, its output is hidden, but while I was testing something, I manually ran the command, and I noticed the following warning:

The directory '/var/useDownloadCache/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.

So I'm afraid we have a problem... I have to go, but I'll try to think of a solution.

[AndrewFarley]

@cgrimal That might be exactly why the CircleCI fails... if you have it failing (I don't see that error on my Mac or Ubuntu) then maybe you can debug and fix it for us? And also to answer your question above, I haven't done tests recently but last time I tested the existing cache-dir stuff didn't work. I'll try to see if I can make it work, and if it does I'll make it a warning if its used. If I can't make it work, I'll make it error out if used.

[AndrewFarley]

@cgrimal Another idea to fix this problem in docker that randomly everyone seems to have, is to let the pip cache be build locally in the docker container, and then moved out from a local path to the mount after pip finishes. I think this may be the best way to solve this problem, because this removed the uid problem completely.

[dschep]

Hey guys, sorry I haven't stayed on top of this discussion I'll try and catch up to review this over the weekend.

[cgrimal]

@AndrewFarley I am not sure I understand your suggestion about the pip cache folder, but what I implemented is quite simple: I use the chmod before the pip command to give ownership to root, run the pip command, and then give ownership back to the user.

What do you think?

I tried to run the tests locally, but some are failing and I'm not sure it is related... (I'm on Ubuntu by the way)

EDIT: The tests are passing on CircleCI! \o/

[sweepy84]

hey @AndrewFarley - started testing, but didnt get very far, unfortunately.

Existing config is:

custom:
  pythonRequirements:
    slim: true
    slimPatterns:
      - ".*testdata.*"
      - "*.exe"
    pythonBin: python # Forced to set this as it fails to find python3.6 executable
    dockerizePip: true

I should note that slim also does not work at all on windows

Test 1 - Installation Only - FAIL

1. I installed from your branch
2. Run 'sls deploy'
   (without changing any options)

Result

Unable to find good bind path format

Test 2 - Without Docker - SUCCESS

1. Removed the following option

dockerizePip: true

2. Run 'sls deploy'

Result
It completely deployment successfully.

Test 3 - With Options - FAIL

1. Add the feature options

useDownloadCache: true
useStaticCache: true

2. Run 'sls deploy'

Result

EPERM: operation not permitted, scandir 'C:\dev\myproject.serverless\requirements'

After doing some research I found this problem with npm . Unfortunately, I tried clearing cache and reinstalling latest npm (v5.6) rerunning, and even using admin privileges in shell, still getting that error.

[sweepy84]

hey @AndrewFarley - any updates on this? my project is getting slower every time I add a library :(

Were the test results helpful?

[farridav]

*Bump I'm also very reliant on this plugin, and have super slow build times, Id love to see this rebased (again) and merged in ...

[AndrewFarley]

@sweepy84 The results were helpful, if only to tell me that I (or someone) needs to get a Windows machine and help this merge request work properly in that environment.

@farridav I've spent a lot of time rebasing this keeping this working with the latest code, but I don't always/continue to have time rebasing this code. This code is in place now in 4 pipelines working very successfully, but all of them are non-windows environments (Linux/OS-X), so this already fixed my problem. I will try to continue to rebase when I have a few moments.

Bottom line is, I, or someone, needs to get a Windows machine (or many of them) to make this work properly on Widnows... or maybe we make Windows support optional for this feature-set? Unfortunately, due to the nature of Docker on Windows, I can't just spin up a VM on the cloud to test this, it needs a raw/real machine, and the highest level of Windows to do this. I neither have the time nor the money to waste nor the machines laying around to do this. Anyone?

[sweepy84]

Hi @AndrewFarley - If you can look past the self-serving nature of this comment, I would strongly recommend that such a fundamental feature of this plugin (or any feature for that matter) by OS agnostic. You would be alienating a very large percentage of developers out there (possibly a majority - certainly my team). You've already done a lot of great work, fixing the windows bugs is well worth the effort in my opinion.

Perhaps @dee-me-tree-or-love can assist with this, given he has already solved the windows problems (#212 / #227 )?

[sweepy84]

@dschep - I'm actually getting a different error now

Unable to find good bind path format

Using:

pythonRequirements:
slim: true
slimPatterns:
- "/test"
- "*.exe"
- "/*.txt"
pythonBin: python # Forced to set this as it fails to find python3.6 executable
useDownloadCache: true
useStaticCache: true
dockerizePip: true or non-linux`

[dee-me-tree-or-love]

@sweepy84 I had been running into the bind path problem on Windows as well - for me the issue was with the volume mounting for the docker-machine - so I just added a path binding option that would satisfy my settings. You can see the path that are attempted in docker.js : L122.

Probably not a solution, but might help 🙃

[AndrewFarley]

Yeah, I worried that might be the case, hmph. Alright, I think I know how to fix it... but I need to get Docker on Windows working. Do any of you have a throwaway/temporary machine I can RDP into to try to debug this? Would save me unnecessary cost of buying Windows Ultimate just to get this feature implemented. @dee-me-tree-or-love / @sweepy84 , or again, maybe someone can step up with Windows and help me fix this in that platform? Let me know.

My goal is/was never to alienate anyone @sweepy84, I just lack the resources to properly test/iterate on that platform. Complain to Microsoft not allowing docker on anything except the most expensive version of windows, and not on a VM as well. :( I tried a few VM providers (aws, gc) docker's not possible on the cloud far as I can tell. If you know how/where, point me in that direction and I'll make it work.

[kichik]

Docker Toolbox is still available and works without Windows 10 Pro and without Hyper-V.

[AndrewFarley]

Hmm, I'll try it out and let you guys know, thanks @kichik

[dschep]

Sorry I can't really provide any help on bindpath issues on windows @sweepy84. I didn't write that feature and don't have the hardware or software licenses to test it 😕

[AndrewFarley]

Alright ladies and gentleman... I've got Windows support fully working. 99% of the way there... @sweepy84 / @kichik / @dee-me-tree-or-love or anyone else with active Windows codebases can you try this? I tested/confirmed all three scenarios work that @sweepy84 reported was failing above. Without docker works, with docker works, with static caching works and it uses the static cache.

All tests are passing, all my active pipelines work with this code (tested against 5 different codebases/pipelines), and I can confirm all the things seem to work fine with Docker Toolbox on Windows 10. Maybe someone else can test with the latest Docker for Windows?

Thanks for any testing/attention, sorry this one has taken forever, since it "works for me" I lost interest in trying to get it merged, and I couldn't justify working on it on a clients dime because it worked for me/us. Hope we're close and can get everyone able to use this soon. And in the near future have both download and static caching enabled by default (because it's amazing). Cheers!

edit One minor bug I found, the download cache isn't quite working on Windows only, but it doesn't fail fatally, it just isn't caching. I'll try to fix it shortly, but this could always be fixed in a future merge as well. :P This is merge ready as far as I'm concerned. :P

[AndrewFarley]

Thought I'd share a success screenshot. Without caching enabled, this codebase takes ~2.5min for the package to build, with caching around 15 seconds, now working on Windows. :P

[farridav]

Nice ! @dschep / @Valian any chance of an approve and merge ?

[sweepy84]

YESSSS!! @AndrewFarley - great job mate! Looks like its working well! oh the time we're going to save!

I saw your edit about not caching in windows only - what do u mean by "Windows only" - looks to be working for me :)

[AndrewFarley]

Re: @sweepy84 there is two kinds of caching, the "static" cache, or the output of pip, and the "download" cache, the intermediary downloads that pip downloads to feed into pip. The static cache is working perfectly on windows, but the download cache doesn't appear to work. But as I said, it's not doing this in a fatal way, it's just keeping the cache in docker from what I can tell. I don't have any more time this weekend to spend on this, I can try again next weekend to fix this feature, or we can merge it and I can fix that in a future patch.

[sweepy84]

@AndrewFarley - oh ok - thanks clarifying.

My 2 cents - makes sense for it to be merged in and fixed at a later time (ideally raise the bug after doing the merge so it's not forgotten) given it's size so you don't waste time rebasing.

[dschep]

Good job on all the hardwork @AndrewFarley, I'll get to reviewing this ASAP. Sometime this week.

[dschep]

Finally got around to reading through all the changes. Amazing work @AndrewFarley. The code looks great and the feature is well designed, and I love the clean-up work you've done, both in the JS & the tests.

I agree that this would be excellent to enable by default. I'm gonna merge this and cut a new release. I'd love a PR updating to make these default behavior and bumping to version 5.0.0-beta0!

[dschep]

I'll publish to NPM on monday when i'm on a computer with npm creds

[dschep]

Released as v4.2.0!!