Skip to content

feat: Support individual packaging with poetry #682

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

Merged
merged 12 commits into from
Mar 14, 2022
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ dist/
downloads/
eggs/
.eggs/
lib/
# lib/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that change needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the source files for this project are located in ./lib/ , this .gitignore was causing any changes to the JS files to be ignored for git purposes. It seemed quite peculiar to me that a project would .gitignore it's own source folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's right - I guess it would be better to just remove this line instead of commenting it

lib64/
parts/
sdist/
Expand Down
4 changes: 3 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,11 @@ class ServerlessPythonRequirements {
if (!isFunctionRuntimePython(arguments)) {
return;
}
const _pyprojectTomlToRequirements = () =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a quite weird pattern to me - could you explain the reasoning behind adding logic like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Briefly, because I needed to reuse the pyprojectTomlToRequirements function and it was originally written to leverage JS this binding.

The functions used in this block:

      return BbPromise.bind(this)
        .then(pipfileToRequirements)
        .then(_pyprojectTomlToRequirements)
        .then(addVendorHelper)
        .then(installAllRequirements)
        .then(packRequirements)
        .then(setupArtifactPathCapturing);

are implemented to use the this binding pattern, where dependencies/args are passed using the this object. Elsewhere in the project, functions are implemented using an explicit pluginInstance context argument.
My understanding is that the former technique is widely considered a legacy JS pattern, whereas the latter is a more acceptable modern practice.

I opted to convert pyprojectTomlToRequirements to use the latter explicit context argument so that I could invoke it from a function that was already being called with such a context (pluginInstance) arg.
In order to make the resulting pyprojectTomlToRequirements compatible with this BbPromise call async chain, I needed to convert the local this instance into a function arg, in addition to passing an empty string for the modulePath argument (since this call context is for the project root directory).
The lambda function I used achieves this adaption.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification - instead of adding this temp var _pyprojectTomlToRequirements let's just add an inline function - I don't think we need this one-of temporary variable

pyprojectTomlToRequirements('', this);
return BbPromise.bind(this)
.then(pipfileToRequirements)
.then(pyprojectTomlToRequirements)
.then(_pyprojectTomlToRequirements)
.then(addVendorHelper)
.then(installAllRequirements)
.then(packRequirements)
Expand Down
23 changes: 7 additions & 16 deletions lib/pip.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const spawn = require('child-process-ext/spawn');
const { quote } = require('shell-quote');
const { buildImage, getBindPath, getDockerUid } = require('./docker');
const { getStripCommand, getStripMode, deleteFiles } = require('./slim');
const { isPoetryProject } = require('./poetry');
const { isPoetryProject, pyprojectTomlToRequirements } = require('./poetry');
const {
checkForAndDeleteMaxCacheVersions,
sha256Path,
Expand Down Expand Up @@ -60,16 +60,9 @@ function generateRequirementsFile(
pluginInstance
) {
const { serverless, servicePath, options, log } = pluginInstance;
if (
options.usePoetry &&
fse.existsSync(path.join(servicePath, 'pyproject.toml')) &&
isPoetryProject(servicePath)
) {
filterRequirementsFile(
path.join(servicePath, '.serverless/requirements.txt'),
targetFile,
pluginInstance
);
const modulePath = path.dirname(requirementsPath);
if (options.usePoetry && isPoetryProject(modulePath)) {
filterRequirementsFile(targetFile, targetFile, pluginInstance);
if (log) {
log.info(`Parsed requirements.txt from pyproject.toml in ${targetFile}`);
} else {
Expand Down Expand Up @@ -566,11 +559,7 @@ function copyVendors(vendorFolder, targetFolder, { serverless, log }) {
* @param {string} fileName
*/
function requirementsFileExists(servicePath, options, fileName) {
if (
options.usePoetry &&
fse.existsSync(path.join(servicePath, 'pyproject.toml')) &&
isPoetryProject(servicePath)
) {
if (options.usePoetry && isPoetryProject(path.dirname(fileName))) {
return true;
}

Expand Down Expand Up @@ -605,6 +594,8 @@ async function installRequirementsIfNeeded(
// Our source requirements, under our service path, and our module path (if specified)
const fileName = path.join(servicePath, modulePath, options.fileName);

await pyprojectTomlToRequirements(modulePath, pluginInstance);

// Skip requirements generation, if requirements file doesn't exist
if (!requirementsFileExists(servicePath, options, fileName)) {
return false;
Expand Down
33 changes: 17 additions & 16 deletions lib/poetry.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,25 @@ const tomlParse = require('@iarna/toml/parse-string');
/**
* poetry install
*/
async function pyprojectTomlToRequirements() {
if (!this.options.usePoetry || !isPoetryProject(this.servicePath)) {
async function pyprojectTomlToRequirements(modulePath, pluginInstance) {
const { serverless, servicePath, options, log, progress } = pluginInstance;

const moduleProjectPath = path.join(servicePath, modulePath);
if (!options.usePoetry || !isPoetryProject(moduleProjectPath)) {
return;
}

let generateRequirementsProgress;
if (this.progress && this.log) {
generateRequirementsProgress = this.progress.get(
if (progress && log) {
generateRequirementsProgress = progress.get(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you validate how that behaves when there's multiple functions packaged individually at the same time? Are they being packaged in parallel or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was not aware of this mechanism. I will try to investigate and get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I conducted an experiment with multiple functions packaged individually. It appears that this plugin is written to package the functions sequentially and does not support parallel packaging:

From installAllRequirements.js:

    for (const f of filteredFuncs) {
      if (!get(f, 'module')) {
        set(f, ['module'], '.');
      }

      // If we didn't already process a module (functions can re-use modules)
      if (!doneModules.includes(f.module)) {
        const reqsInstalledAt = await installRequirementsIfNeeded(
          f.module,
          f,
          this
        );
...

Note how the await installRequirementsIfNeeded is inside of the for loop. This will prevent parallel execution of the packaging of the functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking that 🙇

'python-generate-requirements-toml'
);
generateRequirementsProgress.update(
'Generating requirements.txt from "pyproject.toml"'
);
this.log.info('Generating requirements.txt from "pyproject.toml"');
log.info('Generating requirements.txt from "pyproject.toml"');
} else {
this.serverless.cli.log(
'Generating requirements.txt from pyproject.toml...'
);
serverless.cli.log('Generating requirements.txt from pyproject.toml...');
}

try {
Expand All @@ -42,15 +43,15 @@ async function pyprojectTomlToRequirements() {
'--with-credentials',
],
{
cwd: this.servicePath,
cwd: moduleProjectPath,
}
);
} catch (e) {
if (
e.stderrBuffer &&
e.stderrBuffer.toString().includes('command not found')
) {
throw new this.serverless.classes.Error(
throw new serverless.classes.Error(
`poetry not found! Install it according to the poetry docs.`,
'PYTHON_REQUIREMENTS_POETRY_NOT_FOUND'
);
Expand All @@ -59,16 +60,16 @@ async function pyprojectTomlToRequirements() {
}

const editableFlag = new RegExp(/^-e /gm);
const sourceRequirements = path.join(this.servicePath, 'requirements.txt');
const sourceRequirements = path.join(moduleProjectPath, 'requirements.txt');
const requirementsContents = fse.readFileSync(sourceRequirements, {
encoding: 'utf-8',
});

if (requirementsContents.match(editableFlag)) {
if (this.log) {
this.log.info('The generated file contains -e flags, removing them');
if (log) {
log.info('The generated file contains -e flags, removing them');
} else {
this.serverless.cli.log(
serverless.cli.log(
'The generated file contains -e flags, removing them...'
);
}
Expand All @@ -78,10 +79,10 @@ async function pyprojectTomlToRequirements() {
);
}

fse.ensureDirSync(path.join(this.servicePath, '.serverless'));
fse.ensureDirSync(path.join(servicePath, '.serverless'));
fse.moveSync(
sourceRequirements,
path.join(this.servicePath, '.serverless', 'requirements.txt'),
path.join(servicePath, '.serverless', modulePath, 'requirements.txt'),
{ overwrite: true }
);
} finally {
Expand Down
19 changes: 19 additions & 0 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1452,6 +1452,25 @@ test(
{ skip: !hasPython(3.6) }
);

test(
'poetry py3.6 can package flask with package individually option',
async (t) => {
process.chdir('tests/poetry_individually');
const path = npm(['pack', '../..']);
npm(['i', path]);

sls(['package'], { env: {} });
const zipfiles = await listZipFiles(
'.serverless/module1-sls-py-req-test-dev-hello.zip'
);
t.true(zipfiles.includes(`flask${sep}__init__.py`), 'flask is packaged');
t.true(zipfiles.includes(`bottle.py`), 'bottle is packaged');
t.true(zipfiles.includes(`boto3${sep}__init__.py`), 'boto3 is packaged');
t.end();
},
{ skip: !hasPython(3.6) }
);

test(
'py3.6 can package flask with package individually option',
async (t) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/base/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.1.1.tgz"
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
2 changes: 1 addition & 1 deletion tests/non_build_pyproject/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.1.1.tgz"
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
2 changes: 1 addition & 1 deletion tests/non_poetry_pyproject/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.1.1.tgz"
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
2 changes: 1 addition & 1 deletion tests/pipenv/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.1.1.tgz"
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
2 changes: 1 addition & 1 deletion tests/poetry/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.1.1.tgz"
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
5 changes: 5 additions & 0 deletions tests/poetry_individually/module1/handler.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import requests


def hello(event, context):
return requests.get('https://httpbin.org/get').json()
17 changes: 17 additions & 0 deletions tests/poetry_individually/module1/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
[tool.poetry]
name = "poetry"
version = "0.1.0"
description = ""
authors = ["Your Name <[email protected]>"]

[tool.poetry.dependencies]
python = "^3.6"
Flask = "^1.0"
bottle = {git = "https://[email protected]/bottlepy/bottle.git", tag = "0.12.16"}
boto3 = "^1.9"

[tool.poetry.dev-dependencies]

[build-system]
requires = ["poetry>=0.12"]
build-backend = "poetry.masonry.api"
14 changes: 14 additions & 0 deletions tests/poetry_individually/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"name": "example",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"author": "",
"license": "ISC",
"dependencies": {
"serverless-python-requirements": "file:serverless-python-requirements-5.3.1.tgz"
}
}
32 changes: 32 additions & 0 deletions tests/poetry_individually/serverless.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
service: sls-py-req-test

provider:
name: aws
runtime: python3.6

plugins:
- serverless-python-requirements
custom:
pythonRequirements:
zip: ${env:zip, self:custom.defaults.zip}
slim: ${env:slim, self:custom.defaults.slim}
slimPatterns: ${file(./slimPatterns.yml):slimPatterns, self:custom.defaults.slimPatterns}
slimPatternsAppendDefaults: ${env:slimPatternsAppendDefaults, self:custom.defaults.slimPatternsAppendDefaults}
dockerizePip: ${env:dockerizePip, self:custom.defaults.dockerizePip}
defaults:
zip: false
slimPatterns: false
slimPatternsAppendDefaults: true
slim: false
dockerizePip: false

package:
individually: true

functions:
hello:
handler: handler.hello
module: module1
package:
patterns:
- 'module1/**'