incubator-marvin
incubator-marvin copied to clipboard
apache
Updating Dockerfile to use Python 3
Agree with @Wei-1, and I think we could use the engine itself to save the info about python version (Eg: marvin.ini file) and just use a parameter on the docker file to avoid to maintain two Dockerfiles.
But guys, this is something that we could implement easily in the next version of the toolbox, for now, the user could just customize his own Dockerfile, also all flow of generations uses the python2.7 as default.
All generated files are available to be edited, this feature exists to support all users and environments in a very flexible way.
I think we should design this flexibility in the next version where we going to have a real docker repository available with a bunch of different images to support all kind of environment and clouds. And for now, we should just leave the way it is to avoid to waste time testing the whole flow again.
Well, I agree with both of you. But I believe the default option should be Python 3 right now. Version 2 have only more 6 months of official support: https://pythonclock.org/
So, are we going to move this file to python2-engine/Dockerfile, and create a new python-engine/Dockerfile for python3?
Hi @Wei-1, why move? Why not update the Dockerfile to Python 3 and customize to Python 2 when it has necessary?
When using SageMaker, we can choose from python and python3. And the default python is python2, that's why I think it might get a bit confusing.
Guys, let's make the changes simple to avoid big changes. In this case, the request is to add support to python3.
A simple parameter can solve the problem easily without having to create more files and different flows. The engine-generator task has one parameter called --python lets just use it to change the template of the Dockerfile, customizing the installation. By default, this parameter uses the default python instalation.
In this solution, if we going basically uses the same version the user has, at the same time we give then the flexibility to change the version.
What do you think?
Em sex, 21 de jun de 2019 às 12:36, Wei Chen [email protected] escreveu:
When using SageMaker, we can choose from python and python3. And the default python is python2, that's why I think it might get a bit confusing.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-marvin/pull/27?email_source=notifications&email_token=ABCEIJKI4IZYKGQ4F4GJVZ3P3UUTDA5CNFSM4HX5M4VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYJMXZQ#issuecomment-504548326, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCEIJMV5U6OAPGN72GJCG3P3UUTDANCNFSM4HX5M4VA .
@takabayashi I agree. But if we still use Ubuntu 16.04 image we'll need to install python 3 manually, ok?
Since we are using Docker, maybe we can simply choose an image with python3? Such as python:alpine3.9, which is only 31MB.
@Wei-1 it would be very good! I opened an issue for that, but I believe we need to do some tests because Marvin have many dependencies.
yes like:
apt-get install python3 pip3
Em ter, 25 de jun de 2019 às 11:22, Rafael Novello [email protected] escreveu:
@takabayashi https://github.com/takabayashi I agree. But if we still use Ubuntu 16.04 image we'll need to install python 3 manually, ok?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-marvin/pull/27?email_source=notifications&email_token=ABCEIJNCWLZYJJBQTIJQNH3P4JO6XA5CNFSM4HX5M4VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYREVVI#issuecomment-505563861, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCEIJN7IONY2PICHL7M5K3P4JO6XANCNFSM4HX5M4VA .
@wei-1 I liked your idea also...we should use this in the new version of Marvin.
Em ter, 25 de jun de 2019 às 12:10, Rafael Novello [email protected] escreveu:
@Wei-1 https://github.com/Wei-1 it would be very good! I opened an issue for that, but I believe we need to do some tests because Marvin have many dependencies.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-marvin/pull/27?email_source=notifications&email_token=ABCEIJN7PKP22ADJPMDYGIDP4JUR5A5CNFSM4HX5M4VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYRJEVI#issuecomment-505582165, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCEIJJDMSAJL2MJIJBY7BTP4JUR5ANCNFSM4HX5M4VA .
Hello @rafaelnovello, can you rebase this PR with the latest dev branch? It should solve the CI issue.
You can just git rebase develop in MARVIN-59 branch
or you can pull the develop branch and merge develop to MARVIN-59 branch
Codecov Report
Merging #27 into develop will not change coverage. The diff coverage is
n/a.
@@ Coverage Diff @@
## develop #27 +/- ##
========================================
Coverage 70.92% 70.92%
========================================
Files 26 26
Lines 2198 2198
Branches 226 226
========================================
Hits 1559 1559
Misses 618 618
Partials 21 21
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 6d9d401...64dd814. Read the comment docs.
As stated by Taka, I think setting a variable for Python Version and install different library makes sense.
An easier option is that we can still keep those lines for python2, just comments them out. So that users can un-comment those script when needed without searching for the library to install.
@rafaelnovello, after merging #31, this PR is conflicted to the current DEV branch. Can you please rebase the PR?
Hello @rafaelnovello, please help to follow up for the project to proceed its build. #40 We have to do another re-based PR if you are not able to follow up this week (to 2020-02-16)