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

Make confusion_matrix and roc generic #1285

Merged
merged 1 commit into from
May 8, 2019
Merged

Make confusion_matrix and roc generic #1285

merged 1 commit into from
May 8, 2019
+9 −7

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented May 5, 2019 •
edited by jlewi
Loading

Add s3 and minio storage options in confusion_matrix and roc components.
Frontend s3 support has been added in this PR #1278

This change is Reviewable

@Ark-kun
Copy link
Contributor

Ark-kun commented May 6, 2019

/lgtm
But can this be solved in a more generic way? Can the front-end deduce the storage information?

@k8s-ci-robot k8s-ci-robot assigned Ark-kun and unassigned Ark-kun May 6, 2019
@animeshsingh
Copy link
Contributor

Can minio client not frontend any s3 type storage? Do we need them separately?

@Jeffwan
Copy link
Member Author

Jeffwan commented May 6, 2019

Can minio client not frontend any s3 type storage? Do we need them separately?

Yes. Frontend will check protocol for file in /mlpipeline-ui-metadata.json. It makes calls to minio or S3 correspondingly.

@Jeffwan
Copy link
Member Author

Jeffwan commented May 6, 2019 •
edited
Loading

/lgtm
But can this be solved in a more generic way? Can the front-end deduce the storage information?

I double check code. Actually, storage is not being used for roc and confusion matrix. The only reference for this filed is here for MarkdownViewer.

pipelines/frontend/src/lib/OutputArtifactLoader.ts

Line 202 in 892e308

if (metadata.storage === 'inline') {

Verified it's safe to remove this field on two files. Frontend will get storage service from file path. What do you think?

@Ark-kun
Copy link
Contributor

Ark-kun commented May 7, 2019

I double checks code. Actually, storage is not being used for roc and confusion matrix. The only reference for this filed is here for MarkdownViewer.

pipelines/frontend/src/lib/OutputArtifactLoader.ts

Line 202 in 892e308

if (metadata.storage === 'inline') {

Verified it's safe to remove this field on two files. Frontend will get storage service from file path. What do you think?

/cc @rileyjbauer @gaoning777

@rileyjbauer
Copy link
Contributor

I double check code. Actually, storage is not being used for roc and confusion matrix. The only reference for this filed is here for MarkdownViewer.

pipelines/frontend/src/lib/OutputArtifactLoader.ts

Line 202 in 892e308

if (metadata.storage === 'inline') {

Verified it's safe to remove this field on two files. Frontend will get storage service from file path. What do you think?

Can you clarify this? What two files are you referring to?

Like you mentioned, in OutputArtifactLoader.ts the confusion matrix and ROC code doesn't currently use the output metadata storage field at all, it just uses the source. Does that mean additional frontend changes will be needed after this PR to make use of the now updated storage fields?

@Jeffwan
Copy link
Member Author

Jeffwan commented May 8, 2019 •
edited
Loading

Can you clarify this? What two files are you referring to?

two files I mean are
https://github.com/kubeflow/pipelines/blob/master/components/local/confusion_matrix/src/confusion_matrix.py and
https://github.com/kubeflow/pipelines/blob/master/components/local/roc/src/roc.py

These two files write unneeded storage into manifest.

Like you mentioned, in OutputArtifactLoader.ts the confusion matrix and ROC code doesn't currently use the output metadata storage field at all, it just uses the source. Does that mean additional frontend changes will be needed after this PR to make use of the now updated storage fields?

At the beginning, I think frontend may use storage and then I added different options. Now, after your confirmation, I think we can just remove storage field. Different storage should be supported out-of-box. Frontend doesn't need to make any changes. Otherwise, I think users may have confusion, they are only working with GCS? (I was confused and then file this PR)

@rileyjbauer
Copy link
Contributor

rileyjbauer commented May 8, 2019 •
edited
Loading

Now, after your confirmation, I think we can just remove storage field. Different storage should be supported out-of-box. Frontend doesn't need to make any changes. Otherwise, I think users may have confusion, they are only working with GCS? (I was confused and then file this PR)

I see. So you'll change this PR to just remove storage in these python files to prevent confusion? Sounds good to me.

@Jeffwan Jeffwan force-pushed the make_cm_roc_generic branch from b5d9b76 to f05a1d1 Compare May 8, 2019 22:20
Jeffwan
Jeffwan commented May 8, 2019
View reviewed changes
components/local/confusion_matrix/src/confusion_matrix.py
@@ -40,7 +41,8 @@ def main(argv=None):
'If not set, the input must include a "target" column.')
args = parser.parse_args()

on_cloud = args.output.startswith('gs://')
storage_service_scheme = urlparse.urlparse(args.output).scheme
Copy link
Member Author

Choose a reason for hiding this comment

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

a few cases are

  • minio://dir/file_path
  • gs://dir/file_path
  • s3://dir/file_path
  • local path "/dir/file_path"

scheme for local path is empty, all rest we consider they are on_cloud

@Jeffwan
Copy link
Member Author

Jeffwan commented May 8, 2019 •
edited
Loading

I see. So you'll change this PR to just remove storage in these python files to prevent confusion? Sounds good to me.

Cool. Make the change.

@Ark-kun
Copy link
Contributor

Ark-kun commented May 8, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit cb87b94 into kubeflow:master May 8, 2019
@Jeffwan Jeffwan deleted the make_cm_roc_generic branch May 9, 2019 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@animeshsingh animeshsingh Awaiting requested review from animeshsingh

@gaoning777 gaoning777 Awaiting requested review from gaoning777

@Ark-kun Ark-kun Awaiting requested review from Ark-kun

@hongye-sun hongye-sun Awaiting requested review from hongye-sun

@rileyjbauer rileyjbauer Awaiting requested review from rileyjbauer

Assignees

@Ark-kun Ark-kun

@rileyjbauer rileyjbauer

@hongye-sun hongye-sun

Labels
approved lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jeffwan @Ark-kun @animeshsingh @rileyjbauer @k8s-ci-robot @hongye-sun