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

Minor fix on samples/tfx-oss/README.md #969

Merged
merged 5 commits into from
Apr 26, 2019
Merged

Minor fix on samples/tfx-oss/README.md #969

merged 5 commits into from
Apr 26, 2019

Conversation

ucdmkt
Copy link
Contributor

@ucdmkt ucdmkt commented Mar 14, 2019


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @ucdmkt. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@k8s-ci-robot
Copy link
Contributor

Hi @ucdmkt. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 22, 2019

/ok-to-test

Copy link
Contributor

@neuromage neuromage 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 fix

/ok-to-test
/lgtm

@neuromage
Copy link
Contributor

/hold

Hmm, looks like these changes aren't based on master. I think some of this has already been fixed. Requiring TF 1.12 is a change worth making though. Can you sync and try again? Thanks.

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 22, 2019

BTW, does TFX on Kubeflow requires TF to be installed? Sure, the components use it, but is it needed on the client machine?

@ucdmkt
Copy link
Contributor Author

ucdmkt commented Mar 28, 2019

Without TF installed on client machine, graph compilation failed in my environment.

@Ark-kun
Copy link
Contributor

Ark-kun commented Apr 4, 2019

Without TF installed on client machine, graph compilation failed in my environment.

You should probably also create an issue in https://github.com/tensorflow/tfx .

@neuromage
Copy link
Contributor

Without TF installed on client machine, graph compilation failed in my environment.

You should probably also create an issue in https://github.com/tensorflow/tfx .

I don't think that's needed. This seems to be about instructions for running it with KFP, which belong here.

@vicaire
Copy link
Contributor

vicaire commented Apr 25, 2019

@ucdmkt, is this PR still needed? Can we close?

@ucdmkt
Copy link
Contributor Author

ucdmkt commented Apr 25, 2019

@vicaire I think it is still relevant for the first two bullet points?

@Ark-kun
Copy link
Contributor

Ark-kun commented Apr 25, 2019

I don't think that's needed. This seems to be about instructions for running it with KFP, which belong here.

I do not see any usage of raw TF in the TFX Kubeflow sample:
https://github.com/tensorflow/tfx/blob/master/tfx/examples/chicago_taxi_pipeline/taxi_pipeline_kubeflow.py

Is it the case that TFX requires TF, but does not install it when installed using the wheel?

@ucdmkt
Copy link
Contributor Author

ucdmkt commented Apr 26, 2019

Is it the case that TFX requires TF, but does not install it when installed using the wheel?

Indeed. TFX whl doesn't come with TF. And, TFX documentation instructs users to separately pip install TensorFlow.

(also; updated path to example code according to latest tensorflow/tfx)

$ python tfx/tfx/examples/chicago_taxi_pipeline/taxi_pipeline_kubeflow.py
Traceback (most recent call last):
  File "tfx/tfx/examples/chicago_taxi_pipeline/taxi_pipeline_kubeflow.py", line 21, in <module>
    from tfx.components.evaluator.component import Evaluator
  File "/usr/local/google/home/muchida/miniconda3/envs/tfx-kfp-2/lib/python3.5/site-packages/tfx/components/evaluator/component.py", line 23, in <module>
    from tfx.components.base import base_driver
  File "/usr/local/google/home/muchida/miniconda3/envs/tfx-kfp-2/lib/python3.5/site-packages/tfx/components/base/base_driver.py", line 20, in <module>
    import tensorflow as tf
ImportError: No module named 'tensorflow'

Copy link
Contributor

@neuromage neuromage left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks for fixing this!

@neuromage
Copy link
Contributor

/hold cancel

@neuromage
Copy link
Contributor

@vicaire @IronPan @Ark-kun can one of you please approve? Thanks.

@Ark-kun
Copy link
Contributor

Ark-kun commented Apr 26, 2019

Indeed. TFX whl doesn't come with TF. And, TFX documentation instructs users to separately pip install TensorFlow.

I see. That's rather inconvenient.

@Ark-kun
Copy link
Contributor

Ark-kun commented Apr 26, 2019

@vicaire @IronPan @Ark-kun can one of you please approve? Thanks.

Sure
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 f91ab96 into kubeflow:master Apr 26, 2019
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
updated to add Patterson Consulting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants