You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@submarine.apache.org by GitBox <gi...@apache.org> on 2022/09/28 15:24:14 UTC

[GitHub] [submarine] hhcs9527 opened a new pull request, #999: SUBMARINE-1296. Move seldon-core install to helm

hhcs9527 opened a new pull request, #999:
URL: https://github.com/apache/submarine/pull/999

   ### What is this PR for?
   Move seldon-core install to helm
   
   ### What type of PR is it?
   [Bug Fix]
   
   ### Todos
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/SUBMARINE-1293
   
   ### How should this be tested?
   // TODO
   
   ### Screenshots (if appropriate)
   
   ### Questions:
   * Do the license files need updating? No
   * Are there breaking changes for older versions? No
   * Does this need new documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] asfgit closed pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #999: SUBMARINE-1293. Move seldon-core install to helm
URL: https://github.com/apache/submarine/pull/999


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r984105531


##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -1,3 +1,4 @@
+

Review Comment:
   I have since discovered that deployments based on this file currently have another problem that needs to be solved (https://issues.apache.org/jira/browse/SUBMARINE-1332). I would therefore suggest that we can remove this file for the time being.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on pull request #999: SUBMARINE-1296. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on PR #999:
URL: https://github.com/apache/submarine/pull/999#issuecomment-1261583172

   Hi~ @hhcs9527 
   Thanks your contribution. I've left some comments that may need to be modified and I'll verify the deployment later.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1296. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r982955997


##########
helm-charts/submarine/Chart.yaml:
##########
@@ -23,6 +23,9 @@ version: 0.8.0-SNAPSHOT
 icon: https://submarine.apache.org/assets/themes/submarine/img/submarine_white_logo.png
 dependencies:
   - name: training-operator
-    version: "0.1.0"
+    version: "0.7.0"
   - name: notebook-controller
     version: "0.1.0"
+  - name: seldon-core-operator
+    version: "1.10.0"
+    repository: https://storage.googleapis.com/seldon-charts

Review Comment:
   If the user already has installed `seldon`, I think we can add a condition to make helm not install this dependency. For example:
   ```yaml
   condition: seldon-core-operator.enabled
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] codecov[bot] commented on pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #999:
URL: https://github.com/apache/submarine/pull/999#issuecomment-1279698426

   # [Codecov](https://codecov.io/gh/apache/submarine/pull/999?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#999](https://codecov.io/gh/apache/submarine/pull/999?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2b4cda) into [master](https://codecov.io/gh/apache/submarine/commit/67fe91fde8e15fe757345b744cbff731decac3aa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (67fe91f) will **increase** coverage by `18.60%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master     #999       +/-   ##
   =============================================
   + Coverage     25.46%   44.06%   +18.60%     
   =============================================
     Files           358      112      -246     
     Lines         29078     4679    -24399     
     Branches       3479        0     -3479     
   =============================================
   - Hits           7404     2062     -5342     
   + Misses        21460     2617    -18843     
   + Partials        214        0      -214     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | python-integration | `?` | |
   | python-unit | `44.06% <ø> (-8.12%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/submarine/pull/999?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ine-sdk/pysubmarine/submarine/models/tensorflow.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbW9kZWxzL3RlbnNvcmZsb3cucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pysubmarine/submarine/ml/tensorflow\_v2/model/fm.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9mbS5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ysubmarine/submarine/ml/tensorflow\_v2/model/nfm.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9uZm0ucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...submarine/submarine/ml/tensorflow\_v2/parameters.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9wYXJhbWV0ZXJzLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...submarine/submarine/ml/tensorflow\_v2/registries.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9yZWdpc3RyaWVzLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...bmarine/submarine/ml/tensorflow\_v2/model/deepfm.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9kZWVwZm0ucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...arine/submarine/ml/tensorflow\_v2/input/\_\_init\_\_.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9pbnB1dC9fX2luaXRfXy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...arine/submarine/ml/tensorflow\_v2/model/\_\_init\_\_.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9fX2luaXRfXy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../submarine/ml/tensorflow\_v2/model/base\_tf\_model.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9iYXNlX3RmX21vZGVsLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...submarine/submarine/ml/tensorflow\_v2/model/ccpm.py](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VibWFyaW5lLXNkay9weXN1Ym1hcmluZS9zdWJtYXJpbmUvbWwvdGVuc29yZmxvd192Mi9tb2RlbC9jY3BtLnB5) | `0.00% <0.00%> (-96.78%)` | :arrow_down: |
   | ... and [299 more](https://codecov.io/gh/apache/submarine/pull/999/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1296. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r982949444


##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -1,3 +1,4 @@
+

Review Comment:
   Please ensure that the license information starts on the first line.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r991716319


##########
helm-charts/submarine/templates/seldon-gateway.yaml:
##########
@@ -18,7 +18,7 @@ apiVersion: networking.istio.io/v1alpha3
 kind: Gateway
 metadata:
   name: seldon-gateway
-  namespace: istio-system
+  namespace: default

Review Comment:
   please use  {{ .Release.Namespace }}



##########
helm-charts/submarine/Chart.yaml:
##########
@@ -23,6 +23,10 @@ version: 0.8.0-SNAPSHOT
 icon: https://submarine.apache.org/assets/themes/submarine/img/submarine_white_logo.png
 dependencies:
   - name: training-operator
-    version: "0.1.0"
+    version: "0.7.0"
   - name: notebook-controller
     version: "0.1.0"
+  - name: seldon-core-operator

Review Comment:
   I think we should add the `helm dep update` to https://github.com/apache/submarine/blob/master/website/docs/gettingStarted/quickstart.md and add the dependency description.
   



##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -18,7 +18,7 @@ apiVersion: v1
 kind: Secret
 metadata:
   name: submarine-serve-secret
-  namespace: submarine-user-test
+  namespace: default

Review Comment:
   we should not move and changed this file, please.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r984105531


##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -1,3 +1,4 @@
+

Review Comment:
   I have since discovered that deployments based on this file currently have another problem that needs to be solved (https://issues.apache.org/jira/browse/SUBMARINE-1332). I would therefore suggest that we can remove this file for the time being.
   cc @pingsutw 
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] hhcs9527 commented on a diff in pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
hhcs9527 commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r985245626


##########
submarine-serve/installation/install.sh:
##########
@@ -33,13 +33,3 @@ if [[ ! -f ${CURRENT_PATH}/istioctl ]]; then
 fi
 
 ${CURRENT_PATH}/istioctl install --skip-confirmation
-
-kubectl create ns seldon-system
-kubectl apply -f ${CURRENT_PATH}/seldon-secret.yaml
-helm install seldon-core seldon-core-operator \
-    --repo https://storage.googleapis.com/seldon-charts \
-    --namespace seldon-system \
-    --version 1.10.0 \
-    --set istio.enabled=true \

Review Comment:
   Thanks for the reminding.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] hhcs9527 commented on a diff in pull request #999: SUBMARINE-1293. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
hhcs9527 commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r985245079


##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -1,3 +1,4 @@
+

Review Comment:
   ok, I got it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [submarine] cdmikechen commented on a diff in pull request #999: SUBMARINE-1296. Move seldon-core install to helm

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on code in PR #999:
URL: https://github.com/apache/submarine/pull/999#discussion_r982950114


##########
submarine-serve/installation/install.sh:
##########
@@ -33,13 +33,3 @@ if [[ ! -f ${CURRENT_PATH}/istioctl ]]; then
 fi
 
 ${CURRENT_PATH}/istioctl install --skip-confirmation
-
-kubectl create ns seldon-system
-kubectl apply -f ${CURRENT_PATH}/seldon-secret.yaml
-helm install seldon-core seldon-core-operator \
-    --repo https://storage.googleapis.com/seldon-charts \
-    --namespace seldon-system \
-    --version 1.10.0 \
-    --set istio.enabled=true \

Review Comment:
   We also need to synchronise the variables(`istio.enabled=true` and `executor.defaultEnvSecretRefName=submarine-serve-secret 2> /dev/null`) declared here with `values.yaml` to ensure consistency of configuration.



##########
helm-charts/submarine/templates/seldon-secret.yaml:
##########
@@ -1,3 +1,4 @@
+

Review Comment:
   I've checked this file and found that we need to replace namespace:
   ```yaml
   namespace: {{ .Release.Namespace }}
   ```
   And please ensure that the license information starts on the first line.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org