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

Installing plugin provider bundles unnecessary dependencies into deployment artifact. #2895

Closed
jthomas opened this issue Dec 8, 2016 · 20 comments

Comments

Projects
None yet
7 participants
@jthomas
Copy link
Contributor

commented Dec 8, 2016

This is a (Bug Report / Feature Proposal)

Bug Report.

Description

Provider plugins are now bundled as separate NPM modules and installed using npm install.
This puts the provider module and it's dependencies inside the service's node_modules directory. When the service is packaged for deployment, these additional modules will be included in the zip file.

Here's an example of the openwhisk boilerplate:
https://github.com/jthomas/serverless-openwhisk-boilerplate

Run npm install to see the output in node_modules.

I did try to counter this by manually updating the excludes value to include the module name but with NPM3's module flattening, this doesn't work as all the child dependencies aren't excluded.
https://github.com/serverless/serverless-openwhisk/blob/master/compile/functions/index.js#L21-L25

Looking at the framework's plugin registration mechanism, I was wondering whether this could be managed by having a new command, install-provider, which installs the provider plugins to a new . directory (.serverless-providers). The plugin manager would include this in the search path for plugin registration. The excludes paths would be updated to include this directory.

I'm sure you have better ideas than I do for how to handle this :)

@pmuens

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

Thanks for reporting and giving such a detailed and helpful description (as usual!) @jthomas 👍 🎆

I've faced exactly the same problem during the Google Cloud Functions plugin development (see: serverless/serverless-google-cloudfunctions#5).

Definitely a high priority as it is a usual problem when offering multi provider support through external Serverless plugins.

@mthenw

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2016

Maybe we could use special .serverless_plugins folder?

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2016

Would a short term fix be to add a --provider flag to the install command which changed the output directory for the downloaded artifact to that folder?

@pmuens pmuens removed priority/P1 labels Jan 4, 2017

@pmuens

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Hey @jthomas sorry for the delay here. Let's pick this issue up again! 💯

I think right now the best solution / workaround would be what @mthenw mentioned:
The provider plugin should be installed in the .serverless_plugins directory and then be excluded while packaging.

We need to think this through for a good, stable long term solution as this seems to be an general problem when integrating other providers through plugins.

Here are some possible solutions (summarized from this thread):

  1. Create .serverless_providers directory in the Serverless and let developers install provider plugins with the Serverless CLI
  2. Add a --provider flag to the install command
  3. Put provider plugins in the .serverless_plugins directory and exclude them (no long-term fix) 👎
  4. Add a logic to the packaging mechanism which detects a provider plugin (e.g. based on the naming schema) and excludes it automatically (maybe surprising for the user)
    ...

I'll put this issue to the v1.6 milestone as it's an important feature!

@pmuens pmuens added this to the 1.6 milestone Jan 5, 2017

@pmuens pmuens self-assigned this Jan 5, 2017

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

Ignoring the implementation challenges, I think a combination of 1 & 2 would be the best developer experience.

I can just serverless install --provider openwhisk and this plugin will be imported into .serverless_providers.

@nikgraf

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

@pmuens How would .serverless_providers work? Plugins often come with node_module dependencies or would this directory has it's own node_module dependencies? How does a user then know how to install them?

@jthomas It's an issue for all plugins and we need to find a solution that works also for all languages.
Main question: Is it a blocker to ship the plugin? Would it be okayish to ship it like this?

@pmuens

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Thanks for the response @jthomas and the suggestion!

@nikgraf I would say that the .serverless_providers has it's on node_modules dependencies...

Just wanted to link to this PR #2907 which implemented a plugin discovery and installation process through the CLI.

This could be easily extended (we could e.g. flag plugins as provider and extension plugins in the plugins registry) so that Serverless will automatically move the file in the .serverless_provider directory.

Additionally it would be convenient for the developer / the user to install provider plugins.

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2017

npm install --production on the openwhisk provider plugin installs a node_modules directory with ~22MB.

This is nearly half the maximum deployment package size of 48MB.

It does work as is but will drastically slow down deployments and users will run into size limit issues much faster.

I assumed .serverless_providers directory would have it's own node_modules as @pmuens suggested.

@nikgraf

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2017

Now that I investigated a bit further @pmuens suggestion seems to be the best.

We would to do these things:

  1. Ignore .serverless_plugins when packaged by default (useful for every plugin with Node & Python Packaging). Just add it to this list: https://github.com/serverless/serverless/blob/master/lib/plugins/package/lib/packageService.js#L7-L14
  2. Every template should have a .serverless_plugins/package.json. This is necessary so you can do npm install --save-dev serverless-openwhisk and it doesn't install it in the parent node_modules folder, but rather in .serverless_plugins/node_modules.
  3. We also add .serverless_plugins/node_modules to that module.paths here
    module.paths.unshift(
    path.join(this.serverless.config.servicePath, 'node_modules'),
    path.join(this.serverless.config.servicePath, '.serverless_plugins')
    );

This way people can install plugins without the getting packaged. We don't break the process installing a plugin directly into node_modules, but in the docs we encourage the .serverless_plugins/node_modules way. I would love to go without it, but this seems to be the best solution I could come up with. The change is minimal to implement, but helps all people using plugins without custom packaging.

@pmuens @eahefnawy @jthomas @DavidWells What do you guys think?

@pmuens

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

@nikgraf thanks for the proposal 👍

I like the solution because it doesn't introduce a large rewrite and won't break the existing packaging process. This is only one solution and might not be the prettiest but it does it's thing and provides a quick way to solve the multi-provider plugin issue (we also face with other provider plugins BTW).

Furthermore the whole packaging process benefits from that as we introduce a natural way / best practice how to add dependencies which are not bundled up into the deployment artifact.

I would propose that we go this way so that we have something in place soon.

We can continue the discussion to find a long-term strategy / solution.

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2017

LGTM - good compromise and minimal changes.
Will test this out when it's available.

@pmuens

This comment has been minimized.

Copy link
Member

commented Jan 23, 2017

Great! Thanks @jthomas 🙌

@pmuens

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

Moving this to the next milestone so that we can think it more through and work on this issue there...

@pmuens pmuens modified the milestones: 1.7, 1.6 Jan 30, 2017

@pmuens pmuens removed their assignment Jan 30, 2017

@pmuens pmuens referenced this issue Feb 2, 2017

Merged

Fix node_modules (serverless plugins) lookup #3180

6 of 6 tasks complete
@pmuens

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

@jthomas we got an interesting PR (#3180) which might be helpful for provider plugins and this issue in general...

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2017

Ooo interesting. 🐸.

This would work from a technical perspective.

It does make the developer experience more complex in some circumstances.

For an experienced user, they will probably already have a parent workspace folder with all their service folders. They will be able to add any shared plugins across all these services to this parent directory's node_modules. Works great, mitigating the issue with having to replicate plugins and the bundling issue. 💯

For a new user, who will start with a single service, they might be confused as to why they need to have a parent folder to use the boilerplate templates. This behaviour would not be consistent across providers. I can see people running into the issue. People moving a service from Lambda to OpenWhisk would need to create a new parent folder and make sure the plugin was installed in there.

I think the approach for advanced users who have this project structure is a good idea but a think we'll see new users getting confused and running into issues.

What does everyone think?

I think I'd like both approaches :) One method to be able to share plugins across services but also the best onboarding for new users that reduces the boilerplate steps to get them started.

@nrako

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

@jthomas thanks for your comment. 🤡.

I believe my PR opens up "new" possibilities. One could argue that will add complexity but I don't really think so. That small changes only allow the normal/expected behaviour of node modules loading to work. IMHO working against this well known node principle is counter-intuitive.

For inexperienced user, the documented way of installing plugins can stay the same and maybe leverage behind the scene something like the .serverless_plugins folder or something similar...

I believe more experienced user of JS will quickly look for solutions to transpile & bundle (i.e babel & webpack) their source into an optimised handler.js files with chunks for shared code. Possibly only having native modules built with docker in the node_modules of the service folder to be packaged for deployment.

I'm only starting to play with serverless so I might miss something from the bigger picture...?


By the way I don't really understand the need for new folder conventions .serverless_plugins or .serverless_providers, the package script could simply excludes all devDependencies if a package.json is found no? I'll come with a PR soonish.

@nrako

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

oups about excluding all devDependencies I forgot NPM3 (or Yarn) flattening... I'll think a bit more about this.

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

👋 serverless fw folks.

I'm just checking in to see if this is still on the roadmap? I've had users complain about the complexity of installing the OpenWhisk plugin due to this issue.

The current process is to install the serverless-openwhisk plugin globally and npm link the that into the local node_modules. Installing the plugin locally means the child dependencies get bundled into the deployment package. This is done automatically when running npm install after creating the template but people often forget and run into issues.

Can we look again at making all additional provider plugins first-class citizens of the framework?

I've been re-testing this issue since this comment and it appears that devDepencencies and any child modules are now not included in the deployment package despite module flattening. I'm not sure if NPM or the framework has changed to support this but 👍

I need to test this more but it would resolve the issue completely, hurrah. If so, I'll update the provider instructions and templates and close this issue.

@t-buss

This comment has been minimized.

Copy link

commented Nov 12, 2018

The quickstart guide (https://serverless.com/framework/docs/providers/openwhisk/guide/quick-start/) still references this issue:

Due to an outstanding issue with provider plugins, the OpenWhisk provider must be installed as a global module.

Can one safely ignore this hint or is it still relevant?

@jthomas

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

@t-buss Ignore the docs - I need to update them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.