Skip to content

[Optimization]Optimize some details of MLFlow task plugin #10740#10739

Merged
caishunfeng merged 3 commits intoapache:devfrom
jieguangzhou:dev
Jul 5, 2022
Merged

[Optimization]Optimize some details of MLFlow task plugin #10740#10739
caishunfeng merged 3 commits intoapache:devfrom
jieguangzhou:dev

Conversation

@jieguangzhou
Copy link
Member

@jieguangzhou jieguangzhou commented Jul 2, 2022

Purpose of the pull request

Optimize some details of the MLFlow task plugin

close: #10740

Brief change log

  • Add a health check for deploying the model with docker
  • Change to health Check implemented in Java instead of shell command
  • Modify the test case to improve reading

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

@jieguangzhou jieguangzhou changed the title [Optimization]Optimize some details of MLFlow task plugin [Optimization]Optimize some details of MLFlow task plugin #10740 Jul 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

Codecov Report

Merging #10739 (799a219) into dev (d0f73d3) will decrease coverage by 0.78%.
The diff coverage is 8.00%.

@@             Coverage Diff              @@
##                dev   #10739      +/-   ##
============================================
- Coverage     40.71%   39.93%   -0.79%     
- Complexity     4820     4843      +23     
============================================
  Files           900      902       +2     
  Lines         36143    37270    +1127     
  Branches       3982     4338     +356     
============================================
+ Hits          14717    14885     +168     
- Misses        19972    20910     +938     
- Partials       1454     1475      +21     
Impacted Files Coverage Δ
...nscheduler/plugin/task/mlflow/MlflowConstants.java 0.00% <ø> (ø)
...scheduler/plugin/task/mlflow/MlflowParameters.java 74.22% <0.00%> (-2.86%) ⬇️
...olphinscheduler/plugin/task/mlflow/MlflowTask.java 58.58% <8.33%> (-15.45%) ⬇️
.../server/master/metrics/ProcessInstanceMetrics.java 52.63% <0.00%> (-10.71%) ⬇️
...org/apache/dolphinscheduler/remote/utils/Host.java 43.47% <0.00%> (-2.18%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 51.38% <0.00%> (-1.39%) ⬇️
...nscheduler/service/process/ProcessServiceImpl.java 29.23% <0.00%> (-1.05%) ⬇️
...r/server/master/runner/MasterSchedulerService.java 0.00% <0.00%> (ø)
...duler/server/master/exception/MasterException.java 0.00% <0.00%> (ø)
... and 3 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 d0f73d3...799a219. Read the comment docs.

@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly backend labels Jul 2, 2022
@SbloodyS SbloodyS added this to the 3.0.0-beta-3 milestone Jul 2, 2022
@zhongjiajie
Copy link
Member

restart the failed tests

@jieguangzhou
Copy link
Member Author

I think that will not be released until version 3.1.0-alpha

zhongjiajie
zhongjiajie previously approved these changes Jul 4, 2022
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

…main/java/org/apache/dolphinscheduler/plugin/task/mlflow/MlflowParameters.java
@zhongjiajie
Copy link
Member

@jieguangzhou I change some of your code and you could task a look at #10739 (comment)

@zhongjiajie zhongjiajie modified the milestones: 3.0.0-beta-3, 3.1.0-alpha Jul 4, 2022
@zhongjiajie
Copy link
Member

I think that will not be released until version 3.1.0-alpha

you are right.

@jieguangzhou
Copy link
Member Author

@jieguangzhou I change some of your code and you could task a look at #10739 (comment)

Thanks, your code is better. I learned thaty

@zhongjiajie zhongjiajie self-requested a review July 4, 2022 10:39
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

some nip


}

public int checkDockerHealth() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a general method and can move into ProcessUtils, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't make a sound recommendation because I'm not sure if there are other components that use this function. WDYT @zhongjiajie

@jieguangzhou jieguangzhou requested a review from kezhenxu94 as a code owner July 5, 2022 02:21
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

9.0% 9.0% Coverage
0.0% 0.0% Duplication

@jieguangzhou
Copy link
Member Author

@caishunfeng PTAL, thanks

Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

+1

@caishunfeng caishunfeng merged commit 7d79a21 into apache:dev Jul 5, 2022
@jieguangzhou
Copy link
Member Author

Thanks all

zhongjiajie added a commit to zhongjiajie/dolphinscheduler that referenced this pull request Jul 5, 2022
apache#10739)

* Optimize some details of MLFlow task plugin

* Update dolphinscheduler-task-plugin/dolphinscheduler-task-mlflow/src/main/java/org/apache/dolphinscheduler/plugin/task/mlflow/MlflowParameters.java

* fix some nips

Co-authored-by: Jiajie Zhong <zhongjiajie955@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend improvement make more easy to user or prompt friendly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Optimization]Optimize some details of MLFlow task plugin

5 participants