-
Notifications
You must be signed in to change notification settings - Fork 68
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
updated support for benchmark language versions #198
Conversation
config/systems.json
Outdated
@@ -170,7 +170,6 @@ | |||
}, | |||
"nodejs": { | |||
"base_images": { | |||
"10": "gcr.io/google-appengine/nodejs", | |||
"12": "gcr.io/google-appengine/nodejs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the situation on Azure/GCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the python, nodejs version supports on gcp, azure, openwhisk official documentation most of them supports python versions 3.8 and above and for nodejs 18 and above.
@octonawish-akcodes Hi, any update on my comments? |
Signed-off-by: Abhishek Kumar <[email protected]>
940e85e
to
2f09f26
Compare
@mcopik I updated the PR with the changes, have a look and I would be happy to answer any further queries :) |
@octonawish-akcodes Thank you! On which platforms and languages did you verify it works? We need to run the benchmarks as some of them might break, e.g., due to a different language version required. For example, a common issue is the We can use SeBS regression to simplify the process, e.g., |
Hi, I verfied them against the official documentation of the cloud platforms. Supported links are as follows: Nodejs( https://azure.microsoft.com/en-in/updates/generally-available-azure-functions-support-for-nodejs-18/ ) For GCP: Python (https://devguide.python.org/versions/#supported-versions) For openwhisk Python and Nodejs (https://openwhisk.apache.org/downloads.html) |
Yes, that is a very important first step. However, we need also to execute benchmarks against specified version to check they work in practice. |
I can only try on the Azure platform since I don't have accounts on other platforms |
@octonawish-akcodes I think we can also add Python 3.10 on AWS Lambda. |
I just checked it https://aws.amazon.com/about-aws/whats-new/2023/04/aws-lambda-python-3-10/ I'll add it |
Signed-off-by: Abhishek Kumar <[email protected]>
I was just running benchmarks for python3.10 on AWS, these are the results of the regression test. Some benchmark breaks are due to language version requirements.
|
Can you share the failure, curious to see the traceback @prajinkhadka |
@prajinkhadka 411 is expected, we documented it :-) It just won't work without Docker images due to code size. But please share the error, I'm curious if it's possibly something else. 210 is likely a fixed Pillow version, and 50* is likely a fixed igraph version. |
Most probably it is the issue with the versions as mentioned by Marcin. Regarding the 411 the issue should be with the code size limitation. I will check and update here if it's something else. |
It will be better if u could post the whole traceback here. |
@octonawish-akcodes Here you go. Need to install the correct version of Pillow and igraph for Python3.10 For 50* , traceback from Lambda:
For 210:
|
@prajinkhadka Can you please post the correct versions here - or even better if both you and @octonawish-akcodes can push to the same branch. |
@mcopik Yes, I will be doing it. |
On Azure platform when i try to experiment benchmarks with python version 3.8, I am getting this error and even after exporting the certification using this command
|
Also on running the benchmarks against python version 3.10, all benchmarks are failed to execute.
|
On Azure with python version 3.8 test results: cc @mcopik |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
docs/benchmarks.md (1)
29-29
: LGTM! Fix minor language inconsistency.The added warning about the image-recognition benchmark not working on GCP with Python 3.8+ due to excessive code size is informative and necessary.
However, the phrase "In future" should be corrected to "In the future" to maintain consistency in language.
- In future, we might support Docker-based deployment on AWS that are not affected by code size limitations. + In the future, we might support Docker-based deployment on AWS that are not affected by code size limitations.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.18 (1 hunks)
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.20 (1 hunks)
- benchmarks/400.inference/411.image-recognition/python/requirements.txt.3.10 (1 hunks)
- benchmarks/400.inference/411.image-recognition/python/requirements.txt.3.11 (1 hunks)
- config/systems.json (10 hunks)
- docs/benchmarks.md (1 hunks)
- sebs/azure/azure.py (7 hunks)
- sebs/azure/cli.py (1 hunks)
- sebs/regression.py (2 hunks)
Files skipped from review due to trivial changes (4)
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.18
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.20
- benchmarks/400.inference/411.image-recognition/python/requirements.txt.3.11
- sebs/azure/cli.py
Files skipped from review as they are similar to previous changes (2)
- benchmarks/400.inference/411.image-recognition/python/requirements.txt.3.10
- sebs/regression.py
Additional context used
LanguageTool
docs/benchmarks.md
[locale-violation] ~26-~26: The phrase ‘In future’ is British English. Did you mean: “In the future”?
Context: ...ue to 30 second timeout of API gateway. In future, we might support Docker-based deployme...(IN_FUTURE)
Markdownlint
docs/benchmarks.md
27-27: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
Ruff
sebs/azure/azure.py
209-209: Use context handler for opening files
(SIM115)
Additional comments not posted (9)
config/systems.json (6)
20-21
: LGTM!The addition of Python 3.8 and 3.9 base images for local experiments looks good.
37-38
: LGTM!The addition of Node.js 18 and 20 base images for local experiments looks good.
58-59
: LGTM!The addition of Python 3.11 and 3.10 base images for AWS looks good.
108-109
: LGTM!The addition of Python 3.9 and 3.10 base images for Azure looks good.
127-129
: LGTM!The addition of Node.js 16, 18, and 20 base images for Azure looks good.
200-202
: LGTM!The addition of Python 3.9, 3.10, and 3.11 base images for OpenWhisk looks good.
sebs/azure/azure.py (3)
206-207
: LGTM!The update to the extension bundle version to [4.0.0, 5.0.0) looks good.
219-219
: LGTM!The addition of the container destination parameter to the publish_function looks good.
291-292
: LGTM!The modification to return the destination container path in the update_function looks good.
Update to PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- benchmarks/200.multimedia/210.thumbnailer/python/requirements.txt.3.12 (1 hunks)
- benchmarks/500.scientific/501.graph-pagerank/python/requirements.txt.3.12 (1 hunks)
- benchmarks/500.scientific/502.graph-mst/python/requirements.txt.3.12 (1 hunks)
- benchmarks/500.scientific/503.graph-bfs/python/requirements.txt.3.12 (1 hunks)
- config/systems.json (10 hunks)
- sebs/regression.py (4 hunks)
Files skipped from review due to trivial changes (4)
- benchmarks/200.multimedia/210.thumbnailer/python/requirements.txt.3.12
- benchmarks/500.scientific/501.graph-pagerank/python/requirements.txt.3.12
- benchmarks/500.scientific/502.graph-mst/python/requirements.txt.3.12
- benchmarks/500.scientific/503.graph-bfs/python/requirements.txt.3.12
Files skipped from review as they are similar to previous changes (2)
- config/systems.json
- sebs/regression.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- config/systems.json (10 hunks)
- dockerfiles/gcp/nodejs/Dockerfile.build (2 hunks)
- dockerfiles/nodejs_installer.sh (1 hunks)
Additional comments not posted (16)
dockerfiles/nodejs_installer.sh (3)
3-5
: Verify the definition of$FILE
.Ensure that the variable
$FILE
is defined earlier in the script or passed as an environment variable to avoid potential issues.
4-4
: Confirm the necessity of sourcingnvm.sh
.Sourcing
nvm.sh
is required for Node Version Manager (NVM) to function correctly. Ensure that this file path is correct and that NVM is installed.
6-6
: Ensure the correctness of thecd
command.Verify that the path
/mnt/function
exists and is the intended directory for runningnpm install
. Additionally, consider handling potential errors if the directory does not exist.dockerfiles/gcp/nodejs/Dockerfile.build (4)
4-4
: Verify the setup of NVM.Ensure that the environment variable
NVM_DIR
is correctly set and that the directory/nvm
is the intended location for NVM.
7-7
: Confirm the necessity ofgosu
andwget
.Verify that
gosu
andwget
are necessary for the Docker build process.gosu
is typically used to run commands as a different user, andwget
is used for downloading files.
8-8
: Ensure the NVM installation script URL is correct.Verify that the URL
https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.7/install.sh
is correct and points to the intended version of NVM.
9-9
: Check the correctness of NVM usage commands.Ensure that the commands
. ${NVM_DIR}/nvm.sh && nvm install ${VERSION} && nvm alias default ${VERSION} && nvm use default
correctly set up and use the specified Node.js version.config/systems.json (9)
20-21
: Verify the correctness of Python base images for local experiments.Ensure that the base images
python:3.8-slim
andpython:3.9-slim
are correct and compatible with the local experiments.
37-38
: Verify the correctness of Node.js base images for local experiments.Ensure that the base images
node:18-slim
andnode:20-slim
are correct and compatible with the local experiments.
58-61
: Verify the correctness of Python base images for AWS.Ensure that the base images
amazon/aws-lambda-python:3.11
,amazon/aws-lambda-python:3.10
,amazon/aws-lambda-python:3.9
, andamazon/aws-lambda-python:3.8
are correct and supported by AWS Lambda.
99-101
: Verify the correctness of Python base images for Azure.Ensure that the base images
mcr.microsoft.com/azure-functions/python:3.0-python3.9
,mcr.microsoft.com/azure-functions/python:4-python3.10
, andmcr.microsoft.com/azure-functions/python:4-python3.11
are correct and supported by Azure Functions.
119-121
: Verify the correctness of Node.js base images for Azure.Ensure that the base images
mcr.microsoft.com/azure-functions/node:4-node16
,mcr.microsoft.com/azure-functions/node:4-node18
, andmcr.microsoft.com/azure-functions/node:4-node20
are correct and supported by Azure Functions.
150-153
: Verify the correctness of Python base images for GCP.Ensure that the base images
ubuntu:22.04
for Python versions3.8
,3.9
,3.10
,3.11
, and3.12
are correct and supported by Google Cloud Platform.
171-176
: Verify the correctness of Node.js base images for GCP.Ensure that the base images
ubuntu:18.04
for Node.js versions10
,12
,14
,16
, andubuntu:22.04
for Node.js versions18
and20
are correct and supported by Google Cloud Platform.
199-201
: Verify the correctness of Python base images for OpenWhisk.Ensure that the base images
openwhisk/action-python-v3.9
,openwhisk/action-python-v3.10
, andopenwhisk/action-python-v3.11
are correct and supported by OpenWhisk.
220-221
: Verify the correctness of Node.js base images for OpenWhisk.Ensure that the base images
openwhisk/action-nodejs-v18
andopenwhisk/action-nodejs-v20
are correct and supported by OpenWhisk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- config/systems.json (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- config/systems.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- config/systems.json (10 hunks)
- dockerfiles/local/nodejs/server.js (1 hunks)
- dockerfiles/local/python/Dockerfile.build (1 hunks)
- dockerfiles/local/python/server.py (1 hunks)
- requirements.txt (1 hunks)
- sebs/local/function.py (1 hunks)
- sebs/local/local.py (2 hunks)
Files skipped from review due to trivial changes (2)
- dockerfiles/local/python/Dockerfile.build
- requirements.txt
Files skipped from review as they are similar to previous changes (1)
- config/systems.json
Additional comments not posted (4)
dockerfiles/local/python/server.py (1)
18-18
: Function renamed toprocess_request
.The function
flush_log
was renamed toprocess_request
. Ensure that all references to this function are updated accordingly.dockerfiles/local/nodejs/server.js (1)
12-16
: LGTM! The/alive
route handler is correctly implemented.The new route handler for
/alive
correctly returns a JSON status.sebs/local/function.py (1)
70-73
: LGTM! Theurl
property is correctly implemented.The new
url
property correctly returns the URL of the local function.sebs/local/local.py (1)
226-243
: LGTM! The server availability check increate_function
is correctly implemented.The code correctly waits for the server to become available before proceeding.
@route('/alive', method='GET') | ||
def alive(): | ||
return { | ||
"result:" "ok" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the syntax error in the /alive
route handler.
There is a missing comma in the dictionary returned by the alive
function.
- "result:" "ok"
+ "result": "ok"
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
@route('/alive', method='GET') | |
def alive(): | |
return { | |
"result:" "ok" | |
} | |
@route('/alive', method='GET') | |
def alive(): | |
return { | |
"result": "ok" | |
} |
@octonawish-akcodes @prajinkhadka Thank you for your help! |
Need reviews cc @mcopik
Related issue #196
Summary by CodeRabbit
New Features
/alive
route to Node.js and Python servers that returns a status response.url
property to theFunction
class for better URL handling.Dependency Updates
sharp
dependency for Node.jsthumbnailer
module.pillow
package and specific versions for multiple benchmarks.igraph
package for scientific benchmarks.Enhancements
Documentation
Bug Fixes