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

Refactor sample tests configuration to reduce the efforts of adding samples. #1730

Merged
merged 53 commits into from
Aug 9, 2019
Merged

Refactor sample tests configuration to reduce the efforts of adding samples. #1730

merged 53 commits into from
Aug 9, 2019

Conversation

numerology
Copy link

@numerology numerology commented Aug 4, 2019 •
edited
Loading

Refactor sample tests configuration to reduce the efforts of adding samples.

This change is Reviewable

numerology and others added 24 commits July 23, 2019 15:13
@numerology
Remove redundant import.
e0c7132
Simplify sample_test.yaml by using withItem syntax.
70972db
Merge branch 'master' of https://github.com/kubeflow/pipelines
deb6cc0
Simplify sample_test.yaml by using withItem syntax.
da9a42a
Merge branch 'master' of https://github.com/numerology/pipelines
a203039
Change dict to str in withItems.
81ccbf4
Merge branch 'master' of https://github.com/kubeflow/pipelines
d94a945
Merge branch 'master' of https://github.com/kubeflow/pipelines
bc8a06f
Merge branch 'master' of https://github.com/kubeflow/pipelines
a71515c
remove redundant sed options.
ed7ef51
Fix format/style issues
6b5f8fe
[WIP] Refactor repeated logic into two utility functions.
23448c9
Merge branch 'master' of https://github.com/kubeflow/pipelines into s…
75104ea 
…ample_tests
Merge branch 'master' of https://github.com/kubeflow/pipelines into s…
fbd6470 
…ample_tests

# Conflicts:
#	test/sample-test/run_test.sh
[WIP] Add a utility function to validate the test results from a note…
2272326 
…book test.
[WIP] Refactor test cases (except for notebook sample tests) into ado…
04981d5 
…pting utility functions.

TODO: Need to move the functions of run_*_test.py into a unified run_sample_test.py.
[WIP] Fix a typo in test name and incorporate tfx-cab-classification,…
47b8ee8 
… kubeflow-training-classification, xgboost-training-cm and basic ones into one run_sample_test.py
Fix/add some comments.
10dd1cb
Refactor notebook tests into using utility functions
73e6f3b
lint
d1b7699
Unify naming in sample_test.yaml
c5ea37c
Remove old *_test.py files
d61e607
Merge branch 'master' of https://github.com/kubeflow/pipelines into s…
7951bd6 
…ample_tests
@numerology
Fix tests by fixing test names.
f118d99
@numerology
Copy link
Author

/test kubeflow-pipeline-e2e-test

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 9, 2019
@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 9, 2019

2. Move test-specific environment setup into .py or .ipynb to keep test infra clean.

The test cases may be even moved from the script to individual test case files.
If we had full support for graph components, each test case would just be a task:

component_ref:
    url: https://raw.githubusercontent.com/kubeflow/pipelines/master/samples/core/tfx/taxi-cab-classification-pipeline.component.yaml
arguments:
    project: ml-pipeline-test
    column-names: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/column-names.json
    evaluation: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/eval20.csv
    train: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/train50.csv
    hidden-layer-size: 5
    steps: 5

And maybe the whole test set would be a pipeline:

name: TFX tests
graph:
    tasks:
        test-1:
            component_ref:
                url: https://raw.githubusercontent.com/kubeflow/pipelines/master/samples/core/tfx/taxi-cab-classification-pipeline.component.yaml
            arguments:
                project: ml-pipeline-test
                column-names: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/column-names.json
                evaluation: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/eval20.csv
                train: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/train50.csv
                hidden-layer-size: 5
                steps: 5

        test-2:
            ...

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 9, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 9, 2019
@numerology
Copy link
Author

  1. Move test-specific environment setup into .py or .ipynb to keep test infra clean.

The test cases may be even moved from the script to individual test case files.
If we had full support for graph components, each test case would just be a task:

component_ref:
    url: https://raw.githubusercontent.com/kubeflow/pipelines/master/samples/core/tfx/taxi-cab-classification-pipeline.component.yaml
arguments:
    project: ml-pipeline-test
    column-names: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/column-names.json
    evaluation: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/eval20.csv
    train: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/train50.csv
    hidden-layer-size: 5
    steps: 5

And maybe the whole test set would be a pipeline:

name: TFX tests
graph:
    tasks:
        test-1:
            component_ref:
                url: https://raw.githubusercontent.com/kubeflow/pipelines/master/samples/core/tfx/taxi-cab-classification-pipeline.component.yaml
            arguments:
                project: ml-pipeline-test
                column-names: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/column-names.json
                evaluation: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/eval20.csv
                train: gs://ml-pipeline-dataset/sample-test/taxi-cab-classification/train50.csv
                hidden-layer-size: 5
                steps: 5

        test-2:
            ...

Thanks for the suggestion @Ark-kun . I will take that into consideration for further refactoring and future test design.

@numerology
Copy link
Author

/test kubeflow-pipeline-e2e-test

gaoning777
gaoning777 approved these changes Aug 9, 2019
View reviewed changes
Copy link
Contributor

@gaoning777 gaoning777 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

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

@gaoning777
Copy link
Contributor

Thanks jiaxiao to improve our sample test infra and refactor the codes. This is really important to keep the product quality high and saves huge engineering efforts in the future to add more tests for samples.

@numerology
Copy link
Author

Thanks for your reviews and suggestions! @gaoning777 @Ark-kun

@numerology
Copy link
Author

/test kubeflow-pipeline-e2e-test

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 9, 2019
@numerology
Copy link
Author

Synced with head to fix e2e test. Please relgtm, thx!

@gaoning777
Copy link
Contributor

/lgtm

@numerology
Copy link
Author

/test kubeflow-pipeline-e2e-test

@numerology numerology merged commit b476a84 into kubeflow:master Aug 9, 2019
@numerology numerology mentioned this pull request Aug 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ark-kun Ark-kun Ark-kun left review comments

@gaoning777 gaoning777 gaoning777 approved these changes

Assignees

@Ark-kun Ark-kun

@gaoning777 gaoning777

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

Successfully merging this pull request may close these issues.
4 participants
@numerology @gaoning777 @Ark-kun @k8s-ci-robot