Skip to content

Allow ability to override slim defaults #216

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
mikepm35 opened this issue Jul 5, 2018 · 8 comments · Fixed by #276
Closed

Allow ability to override slim defaults #216

mikepm35 opened this issue Jul 5, 2018 · 8 comments · Fixed by #276

Comments

@mikepm35
Copy link

mikepm35 commented Jul 5, 2018

(Note: I am packaging individually with zip: true, and am using a branch from @felipe-augusto to deal with the issue as described in #203.)

In order to meet lambda size limitations I am using slim: true, however I was not able to successfully run due to one my modules (Sagemaker SDK) not being able to find a dependency (in this case urllib3). What I found is that Sagemaker is using pkg_resources to pull the SDK version (below), and then pkg_resources throws an exception because it is not able to identify the distribution information since all of the dist-info files have been removed by slim.

SDK_VERSION = pkg_resources.require('sagemaker')[0].version

Setting slim: false is not an option for me since I need that additional trimming, but there is no easy workaround since custom slim options only append to the defaults. I could create specialized packages that I trim myself, or create a branch that removes that default option. I went with the latter, but I think it would be a handy feature to allow for overriding the defaults. If defining custom slim options, it might be easier to remove the defaults altogether, and if a user wants them they can add them back in manually or use a special default option.

@dschep
Copy link
Contributor

dschep commented Jul 10, 2018

Huh. Is the issue a file that's remove or a file that's striped? If it's the former, I think you should be able to use the slimPatterns option to specify a custom set of patterns to be deleted?

@johnf
Copy link

johnf commented Sep 6, 2018

I have a similar issue with strip'd files breaking. e.g. opencv
gives the following when slim is enabled

Unable to import module 'handler': /tmp/sls-py-req/cv2/cv2.so: ELF load command address/offset not properly aligned

@dschep
Copy link
Contributor

dschep commented Sep 6, 2018

What OS are you on and are you using docker? @johnf

@bweigel
Copy link
Contributor

bweigel commented Nov 7, 2018

I have found the same problem using sqlalchemy with the sqlalchemy_redshift plugin/dialect.
pkg_resources is used internally to find the plugins, which itself uses egg-info and dist-info to determine some information.

Huh. Is the issue a file that's remove or a file that's striped? If it's the former, I think you should be able to use the slimPatterns option to specify a custom set of patterns to be deleted?

Unfortunately no, because of the way slimPatterns is implemented it concatenates to the list instead of overwriting:

  let patterns = ['**/*.py[c|o]', '**/__pycache__*', '**/*.dist-info*'];
  if (options.slimPatterns) {
    patterns = patterns.concat(options.slimPatterns);
}

@mikepm35
Copy link
Author

mikepm35 commented Nov 7, 2018

Maybe an solution would be to create a new customPatterns option that would remove all defaults to give complete control.

@dschep
Copy link
Contributor

dschep commented Nov 7, 2018

Tha would be reasonable @mikepm35

@bweigel
Copy link
Contributor

bweigel commented Nov 8, 2018

I see the point, however I would argue one doesn't need an extra option (i.e. customPatterns) in addition to slimPatterns, because you wouldn't use them at the same time (i.e. you either want to overwrite or not).

I propose to use a flag for the overwrite option, which let's you disable (or rather enable overwriting) the default patterns. This way backwards compatibility is also kept. I have updated my PR accordingly.

Cheers.

@dschep
Copy link
Contributor

dschep commented Nov 8, 2018

Yup. no worries, i'm accepting your change @bweigel. @mikepm35 & others, see #276 for how this will be implemented.

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 a pull request may close this issue.

4 participants