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.

Sign up for GitHub

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

Jump to bottom

[Feature] Enable users to create python env from requirements.txt #10658

Merged
merged 6 commits into from
Jul 6, 2022
Merged

[Feature] Enable users to create python env from requirements.txt #10658

merged 6 commits into from
Jul 6, 2022

Conversation

EricGao888
Copy link
Member

@EricGao888 EricGao888 commented Jun 28, 2022 •
edited
Loading

Purpose of the pull request

Brief change log

  • Already described above.

Verify this pull request

  • Verified by manual tests and UTs.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022 •
edited
Loading

Codecov Report

Merging #10658 (fa297b0) into dev (247ca4a) will decrease coverage by 0.27%.
The diff coverage is 23.69%.

❗ Current head fa297b0 differs from pull request most recent head eab04fd. Consider uploading reports for the commit eab04fd to get more accurate results

@@             Coverage Diff              @@
##                dev   #10658      +/-   ##
============================================
- Coverage     40.89%   40.62%   -0.28%     
+ Complexity     4830     4820      -10     
============================================
  Files           897      912      +15     
  Lines         36157    36257     +100     
  Branches       4006     3995      -11     
============================================
- Hits          14788    14731      -57     
- Misses        19898    20060     +162     
+ Partials       1471     1466       -5
Impacted Files Coverage Δ
...duler/api/service/impl/AccessTokenServiceImpl.java 83.72% <ø> (ø)
...scheduler/api/service/impl/ClusterServiceImpl.java 88.28% <ø> (ø)
...cheduler/api/service/impl/ExecutorServiceImpl.java 40.31% <ø> (ø)
...r/api/service/impl/ProcessInstanceServiceImpl.java 60.10% <ø> (ø)
...i/service/impl/ProcessTaskRelationServiceImpl.java 21.03% <ø> (ø)
...scheduler/api/service/impl/ProjectServiceImpl.java 57.92% <ø> (ø)
...heduler/api/service/impl/ResourcesServiceImpl.java 48.56% <ø> (ø)
...heduler/api/service/impl/SchedulerServiceImpl.java 8.36% <ø> (ø)
...scheduler/api/service/impl/SessionServiceImpl.java 78.94% <ø> (ø)
...er/api/service/impl/TaskDefinitionServiceImpl.java 23.96% <ø> (ø)
... and 89 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 247ca4a...eab04fd. Read the comment docs.

@SbloodyS SbloodyS added feature new feature backend labels Jun 29, 2022
@SbloodyS SbloodyS added this to the 3.1.0-alpha milestone Jun 29, 2022
@EricGao888 EricGao888 marked this pull request as ready for review June 29, 2022 17:13
@EricGao888
Copy link
Member Author

@zhongjiajie Could u plz take a look when available? Thx~

@zhongjiajie
Copy link
Member

zhongjiajie commented Jul 1, 2022 •
edited
Loading

I wonder, could we add this feature in python task and jupyter task extend from it? Is it possible?

@EricGao888
Copy link
Member Author

I wonder, could we add this feature in python task and jupyter task extend from it? Is it possible?

Sounds cool to me. I think we may abstract a python kind task, so that all specific kinds of python related tasks could extend it, as well as all its approaches to manage python environments. Currently, the three approaches of python env management are experimental. I suggest we only put them in jupyter task first and see what feedback we could get from users. If positive, we may take the next step and abstract the python kind task mentioned above.

WDYT @zhongjiajie

@EricGao888
Copy link
Member Author

BTW, although I stick to using conda, venv looks pretty light-weighted and do not need users to install Anaconda manually. Maybe we could take venv into consideration in the future work.

@SbloodyS
Copy link
Member

SbloodyS commented Jul 1, 2022

BTW, although I stick to using conda, venv looks pretty light-weighted and do not need users to install Anaconda manually. Maybe we could take venv into consideration in the future work.

I think we can support both of them if you can.

@EricGao888
Copy link
Member Author

EricGao888 commented Jul 1, 2022 •
edited
Loading

BTW, although I stick to using conda, venv looks pretty light-weighted and do not need users to install Anaconda manually. Maybe we could take venv into consideration in the future work.

I think we can support both of them if you can.

I'd love to. Will do some experiments on venv later this month.

@EricGao888
Copy link
Member Author

EricGao888 commented Jul 5, 2022 •
edited
Loading

May I ask whether there are any follow-up comments on this PR? Thx~

zhongjiajie
zhongjiajie reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

other LGTM

...-task-jupyter/src/main/java/org/apache/dolphinscheduler/plugin/task/jupyter/JupyterTask.java Outdated
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

should avoid using *

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, PTAL @zhongjiajie Thx~

@EricGao888 EricGao888 mentioned this pull request Jul 6, 2022
Closed
3 tasks
@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

94.7% 94.7% Coverage
0.0% 0.0% Duplication

zhongjiajie
zhongjiajie approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@zhongjiajie zhongjiajie changed the title [Feature][Task Plugin] Enable users to create python env from requirements.txt [Feature] Enable users to create python env from requirements.txt Jul 6, 2022
@zhongjiajie zhongjiajie merged commit 71f0168 into apache:dev Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

@Tianqi-Dotes Tianqi-Dotes Awaiting requested review from Tianqi-Dotes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

Assignees

@EricGao888 EricGao888

Labels
backend feature new feature
Projects
DolphinScheduler Roadmap
Status: Done
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

[Feature][Task Plugin] Enable users to create python env from requirements.txt
4 participants
@EricGao888 @codecov-commenter @zhongjiajie @SbloodyS