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

TFX Iris sample #3119

Merged
merged 25 commits into from
Mar 14, 2020
Merged

TFX Iris sample #3119

merged 25 commits into from
Mar 14, 2020

Conversation

numerology
Copy link

@numerology numerology commented Feb 19, 2020

Illustrative purpose

DO NOT SUBMIT Changed my mind. I think we should include this sample :)

Verified at https://3ed47013582d1554-dot-us-central2.pipelines.googleusercontent.com/#/runs/details/a17f06a2-7799-4ad0-9bde-bc9c275b6355

Environment:
SDK: TFX 0.21.0 + KFP 0.2.4
Runtime: Hosted pipeline 0.2.3

  • Sample test coverage
  • Prebuilt sample config

This change is Reviewable

@numerology
Copy link
Author

/cc @jingzhang36
FYI

@numerology numerology changed the title [WIP] TFX Iris sample TFX Iris sample Feb 19, 2020
from tfx.components import Trainer
from tfx.components import Transform
from tfx.components.base import executor_spec
from tfx.components.trainer.executor import GenericExecutor
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what would be the best option when we want to import executors from multiple components.

Copy link
Author

Choose a reason for hiding this comment

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

Ummm. That's a good question. I don't see any shortcut to that.

I guess partly that's because we don't often explicitly interact with TFX component executor when authoring the pipeline.

@Ark-kun
Copy link
Contributor

Ark-kun commented Feb 20, 2020

/lgtm

@jingzhang36
Copy link
Contributor

I have a question regarding the Evaluator output of this pipeline. I did a run of this pipeline and the Evaluator's output in my public GCS bucket gs://jingzhangjz-project-outputs/tfx_iris/8d8d58b1-2826-4533-ae0d-c04a65b2864d/Evaluator/evaluation/30.

While I used the following to parse the metric file under that bucket

import tensorflow as tf
from tensorflow_model_analysis.proto import metrics_for_slice_pb2
from typing import Any
for record in tf.compat.v1.python_io.tf_record_iterator('gs://jingzhangjz-project-outputs/tfx_iris/8d8d58b1-2826-4533-ae0d-c04a65b2864d/Evaluator/evaluation/30/metrics'):
metrics_for_slice = metrics_for_slice_pb2.MetricsForSlice.FromString(record)
print(metrics_for_slice)

The parse data seems missing metrics field. They are like
slice_key {
single_slice_keys {
column: "sepal_length"
float_value: 4.900000095367432
}
}
slice_key {
single_slice_keys {
column: "sepal_length"
float_value: 5.699999809265137
}
}
slice_key {
...
}
....

There are all slice_key fields, but no metrics field. As a reference, the taxi example in KFP has parsed data that looks like:
slice_key{
...
}
metrics {
key: "precision"
value {
double_value {
}
}
}
metrics {
}

@numerology
Copy link
Author

There are all slice_key fields, but no metrics field.

It seems like a Keras problem IIUC. Switching to vanilla TF solved this.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 20, 2020
@numerology
Copy link
Author

There are all slice_key fields, but no metrics field.

It seems like a Keras problem IIUC. Switching to vanilla TF solved this.

And if you indeed want to use Keras, you'll need to handcraft the eval_config like following:

eval_config = tfma.EvalConfig(
      model_specs=[
          tfma.ModelSpec(name='candidate', label_key='variety'),
          tfma.ModelSpec(
              name='baseline', label_key='variety', is_baseline=True)
      ],
      slicing_specs=[tfma.SlicingSpec()],
      metrics_specs=[
          tfma.MetricsSpec(metrics=[
              tfma.MetricConfig(
                  class_name='SparseCategoricalAccuracy',
                  threshold=tfma.config.MetricThreshold(
                      value_threshold=tfma.GenericValueThreshold(
                          lower_bound={'value': 0.9}),
                      change_threshold=tfma.GenericChangeThreshold(
                          direction=tfma.MetricDirection.HIGHER_IS_BETTER,
                          absolute={'value': -1e-10})))
          ])])

@jingzhang36
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 21, 2020
@numerology
Copy link
Author

/assign @rmgogogo

@numerology
Copy link
Author

@rmgogogo , this is the forked native Keras sample.

@neuromage
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@numerology
Copy link
Author

/retest

2 similar comments
@numerology
Copy link
Author

/retest

@numerology
Copy link
Author

/retest

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@numerology
Copy link
Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7930f83 into kubeflow:master Mar 14, 2020
@numerology numerology deleted the iris-sample branch March 14, 2020 00:48
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
* init

* update comment

* fix module file

* clean up

* update to beam sample

* add doc of default bucket

* bump viz server tfma version

* update iris sample to keras native version

* update iris sample to keras native version

* pin TFMA

* add readme

* add to sample test corpus

* add prebuilt && update some config

* sync frontend

* update snapshot

* update snapshot

* fix gettingstarted page

* fix unit test

* fix unit test

* update description

* update some comments

* add some dependencies.
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.

6 participants