Skip to content

noDeploy defaults lost when defining overrides #186

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

Closed
JBartlett86 opened this issue May 2, 2018 · 15 comments
Closed

noDeploy defaults lost when defining overrides #186

JBartlett86 opened this issue May 2, 2018 · 15 comments
Labels

Comments

@JBartlett86
Copy link

Hi,

On setting some additional noDeploy overrides within my configuration I noticed my deployed package instantly began to bloat in size. It turns out that by setting an override all the core default are lost.

Defaults:

noDeploy: [
          'boto3',
          'botocore',
          'docutils',
          'jmespath',
          'python-dateutil',
          's3transfer',
          'six',
          'pip',
          'setuptools'
        ]

This means you begin to include things like boto3 within your deployment package unless you replicate the overrides defined above in your serverless yaml (in addition to your specific overrides).

This seems a bit wrong as you will never want to deploy the above libraries and it would seem to make more sense to include the noDeploy overrides specified within the serverless.yaml as an override to this list not as a replacement for it?

Thanks,
John

@petergaultney
Copy link

+1

Also, I am noticing that noDeploy doesn't appear to do anything for me (e.g. boto and botocore are in my .requirements.zip. Is this because I'm also using zip: true, or could it be related to dockerizePip: 'non-linux'?

@dschep
Copy link
Contributor

dschep commented Sep 20, 2018

Could you both share your relevant sections of serverless.yml? noDeploy has defaults and setting it in your config sets it, it doesn't add to it. This is because otherwise, you wouldn't be able to remove anything from it. So, if you want to not include any of the faults AND say, flask, your config should have:

custom:
  pythonRequirements:
    noDeploy: 
      - boto3
      - botocore
      - docutils
      - jmespath
      - python-dateutil
      - s3transfer
      - six
      - pip
      - setuptools
      - flask

@Laurian
Copy link

Laurian commented Sep 23, 2018

@petergaultney I have the same issue, zip: true looks like it isn't affected by noDeploy :(

@Laurian
Copy link

Laurian commented Sep 23, 2018

OK, based on #138 (comment) I managed to remove things from .requirements.zip with serverless-scriptable-plugin like this:

custom:
  pythonRequirements:
    dockerizePip: true
    zip: true
    noDeploy:
      - boto3
      - botocore
  scriptHooks:
    before:package:createDeploymentArtifacts:
      - "zip -d .requirements.zip boto3/* botocore/* || echo ERR: UNABLE TO REMOVE FROM ZIP"

@mrtj
Copy link

mrtj commented Oct 16, 2018

@dschep indeed I use this plugin to prepare my greengrass lambda functions and i DO need boto3, botocore, etc. embedded in my package as they are not available by default in the greengrass lambda environment

@dschep dschep added the bug label Oct 17, 2018
@dschep
Copy link
Contributor

dschep commented Oct 17, 2018

Thanks for the reports & work around. I'll take a look at this tonight.

@mrtj, interesting. I'd never heard of greengrass.

@johnbartlettapak
Copy link

Hi,

I was just wondering whether there had been any movement on;

  1. Keeping the default set of exclusions even after setting the noDeploy parameter
  2. Removing dependencies that are in the noDeploy but are brought in by other requirements - For the moment I could just use the serverless-scriptable-plugin solution above but am interested to know if you are planning on implementing a solution?

Thanks,
John

dschep added a commit that referenced this issue Nov 7, 2018
@dschep
Copy link
Contributor

dschep commented Nov 7, 2018

@Laurian, I just added a test.. AFAICT zip & noDeploy work fine together. See 4447b28

@johnbartlettapak,

  1. does noDeploy defaults lost when defining overrides #186 (comment) not work for you?
  2. hadn't even considered that. It's not on the road map. It goes counter to what I know about python packaging, so I'm not super comfortable doing something like that. If package A you depend on doesn't actually depend on package B then package B shouldn't be a dependency of package A! If it's an optional dependency, that's what extras are for. I had a somewhat similar issue with records depending on pandas even tho it isn't actually a hard dependency. I fixed this by making a PR upstream to fix the problem instead of packaging hacks: Make pandas support optional, as it is with tablib kennethreitz/records#113

@johnbartlettapak
Copy link

johnbartlettapak commented Nov 9, 2018

@dschep

  1. To fix the issue with the overrides I didn't need to use the plugin as I just had to re-enter them - so include boto3 within my noDeploy list worked.
    This just feels a bit wrong as why should I reenter boto3 just because I now want to exclude pytest?

  2. I think this issue specifically relates to boto/boto3 libraries - I am using smart_open which depends on boto3 so when you trim boto3 out of the requirements.txt it gets brought in anyway when smart_open is resolved.
    Now the serverless-scriptable-plugin does seem to fix the problem by allowing me to strip those plugins out but is this something you envision being part of your plugin?

Thanks,
John

@dschep
Copy link
Contributor

dschep commented Nov 10, 2018

Ah, I see your point for the 2nd one now. makes sense for things like boto3.

bweigel pushed a commit to bweigel/serverless-python-requirements that referenced this issue Nov 17, 2018
bweigel pushed a commit to bweigel/serverless-python-requirements that referenced this issue Nov 17, 2018
@DenisStad
Copy link

DenisStad commented Mar 14, 2019

I don't think zip: true and noDeploy work together. I see you added a test @dschep, but if I have zip: true the removed requirements are in the package, if I set it to false or comment it out they are gone. I'm checking by hand.

I believe it's because injectRequirements(https://github.com/UnitedIncome/serverless-python-requirements/blob/master/lib/inject.js#L37) is filtering the requirements if zip is false (https://github.com/UnitedIncome/serverless-python-requirements/blob/master/lib/inject.js#L119), but for the zipped requirements if zip is true, nothing is filtered, which should happen here https://github.com/UnitedIncome/serverless-python-requirements/blob/master/lib/zip.js#L94.

Update:
I think I found out why your test works: If the requirement is explicitly listed in requirements.txt, it works as intended and the removed requirement is not in the final package. In my case, I tried to remove a requirement that was not explicitly listed, but installed by pip because another requirement depended on it.

@radove
Copy link

radove commented Apr 18, 2019

Hello, I had a similar issue but I was trying to achieve the opposite. When I have zip: true and I have a noDeploy of [], a newer boto3 version would get rolled up in the requirements.zip, but when the Lambda ran, it was running the "default" boto3 version that comes with the Lambda. In order to fix this issue, when I comment out the zip: true line, it allowed me to utilize my local boto3 version from Pipfile (and from the requirements.zip/txt). This issue gave me the hint of commenting out the zip: true line to have success with overriding AWS's boto3 default.

@blackmamo
Copy link
Contributor

I just added this PR #354. But I also notice this issue #304. Not sure what way the maintainers want to go, but please consider it

@zbyte64
Copy link

zbyte64 commented Aug 4, 2019

I am still having issues with this. What I have seen is that the tests cover removal of an optional package: flask and the inclusion of a core package boto3. Is there a test of noDeploy on boto3 or botocore (or other core packages)?

I have tried combinations of zip & dockerizePip

serverless: 1.48.4
serverless-python-requirements: 4.3.0

@dschep
Copy link
Contributor

dschep commented Aug 8, 2019

closing since in v5 there are no defaults, per aws recomendations for pckaging

@dschep dschep closed this as completed Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants