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.

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

fix: Updating regular Docker Images for helm chart. #885

Merged
merged 5 commits into from
Aug 19, 2020

Conversation

WaterKnight1998
Copy link
Contributor

I tried to use your regular images. However, I found that regular ones are not up to date. When you try to build these images you get lot of errors.

So this pull request updates those images.

@welcome
Copy link

welcome bot commented Jun 30, 2020

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. This helps us to create release messages and credit you for your hard work!
Examples of commit messages with semantic prefixes:

  • fix: Fix LightGBM crashes with empty partitions
  • feat: Make HTTP on Spark back-offs configurable
  • docs: Update Spark Serving usage
  • build: Add codecov support
  • perf: improve LightGBM memory usage
  • refactor: make python code generation rely on classes
  • style: Remove nulls from CNTKModel
  • test: Add test coverage for CNTKModel

Make sure to check out the developer guide for guidance on testing your change.

@ghost
Copy link

ghost commented Jun 30, 2020

CLA assistant check
All CLA requirements met.

@WaterKnight1998 WaterKnight1998 changed the title Updating regular Docker Images from helm chart. fix: Updating regular Docker Images from helm chart. Jun 30, 2020
@WaterKnight1998 WaterKnight1998 changed the title fix: Updating regular Docker Images from helm chart. fix: Updating regular Docker Images for helm chart. Jun 30, 2020
Copy link
Collaborator

@mhamilton723 mhamilton723 left a comment

Choose a reason for hiding this comment

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

Thanks for the heads up, looping in the right folks to look at this

tools/helm/livy/Dockerfile Outdated Show resolved Hide resolved
@mhamilton723
Copy link
Collaborator

@dbanda can you take a look at this?

Copy link
Contributor

@dbanda dbanda left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. It's been a while since I update this code. My suggestion would be to remove fbprophet from the dockerfiles since it isn't as essentials as numpy, pandas. Users can install fbprophet on their own.

tools/helm/zeppelin/Dockerfile Outdated Show resolved Hide resolved
tools/helm/livy/Dockerfile Outdated Show resolved Hide resolved
@WaterKnight1998
Copy link
Contributor Author

@dbanda and @mhamilton723 I removed fbprophet, it was there because I built those images for a personal project.

Should I add my name as maintainer?

@dbanda
Copy link
Contributor

dbanda commented Jul 7, 2020

@WaterKnight1998 to keep things simple and secure we keep the maintainers as members of the mmlspark team.

@WaterKnight1998
Copy link
Contributor Author

@WaterKnight1998 to keep things simple and secure we keep the maintainers as members of the mmlspark team.

Solved @dbanda

@dbanda and @mhamilton723 it is ready for merge!!

@WaterKnight1998
Copy link
Contributor Author

@dbanda & @mhamilton723 have you checked it?

@WaterKnight1998
Copy link
Contributor Author

@mhamilton723 & @dbanda did you forgive it?

@dbanda
Copy link
Contributor

dbanda commented Jul 28, 2020

LGTM

@mhamilton723
Copy link
Collaborator

Thanks @WaterKnight1998 ill start the build

@mhamilton723
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #885 into master will decrease coverage by 7.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
- Coverage   85.26%   78.15%   -7.12%     
==========================================
  Files         189      189              
  Lines        8769     8769              
  Branches      558      558              
==========================================
- Hits         7477     6853     -624     
- Misses       1292     1916     +624     
Impacted Files Coverage Δ
...ql/execution/streaming/continuous/HTTPSinkV2.scala 0.00% <0.00%> (-96.56%) ⬇️
.../execution/streaming/continuous/HTTPSourceV2.scala 0.00% <0.00%> (-93.05%) ⬇️
...che/spark/sql/execution/streaming/HTTPSource.scala 0.00% <0.00%> (-87.81%) ⬇️
...ql/execution/streaming/DistributedHTTPSource.scala 0.00% <0.00%> (-84.36%) ⬇️
.../scala/com/microsoft/ml/spark/io/IOImplicits.scala 10.52% <0.00%> (-64.92%) ⬇️
...he/spark/sql/execution/streaming/ServingUDFs.scala 0.00% <0.00%> (-52.95%) ⬇️
...om/microsoft/ml/spark/io/http/PortForwarding.scala 0.00% <0.00%> (-41.03%) ⬇️
...la/com/microsoft/ml/spark/io/http/HTTPSchema.scala 51.42% <0.00%> (-37.15%) ⬇️
...microsoft/ml/spark/core/schema/SparkBindings.scala 69.23% <0.00%> (-30.77%) ⬇️
.../microsoft/ml/spark/core/env/StreamUtilities.scala 81.48% <0.00%> (-3.71%) ⬇️

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 54a623d...22aa3fb. Read the comment docs.

@WaterKnight1998
Copy link
Contributor Author

Thanks @WaterKnight1998 ill start the build

Thank you!!

@WaterKnight1998
Copy link
Contributor Author

@mhamilton723 @dbanda did you forgive it? Could you merge it, pelase? It has been more than a month...

@mhamilton723 mhamilton723 merged commit b431a61 into microsoft:master Aug 19, 2020
@welcome
Copy link

welcome bot commented Aug 19, 2020

Congrats on merging your first pull request, we appreciate your support! 🎉🎉🎉

@mhamilton723
Copy link
Collaborator

@WaterKnight1998 sorry for the delay just merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants