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

Components - Added more GCP BigQuery components #3914

Merged
merged 6 commits into from
Jun 26, 2020
Merged

Components - Added more GCP BigQuery components #3914

merged 6 commits into from
Jun 26, 2020

Conversation

NikeNano
Copy link
Member

@NikeNano NikeNano commented Jun 4, 2020

Update the kubeflow GCP components for BigQuery to separate different use cases. The updates are related to this issue #2640

The goal of this PR is to split it up to three different components:

bigquery_query_to_gcs_op - write data to a gcp bucket(and a table)
biqquery_query_op - execute the query
biqquery_query_to_table - write the results to a table in BigQuery

@kubeflow-bot
Copy link

This change is Reviewable

@NikeNano NikeNano force-pushed the components_update branch from d4a2b14 to 1fd97eb Compare June 4, 2020 14:21
@NikeNano
Copy link
Member Author

NikeNano commented Jun 4, 2020 •
edited
Loading

I am a bit unsure what we hope to accomplish with the bigquery_query_opbased upon the discussion in the issue but gave it my best effort related to what I understood from the thread.

@NikeNano
Copy link
Member Author

NikeNano commented Jun 4, 2020

/assign @Ark-kun

@NikeNano
Copy link
Member Author

friendly ping @Ark-kun :)

@NikeNano
Copy link
Member Author

@hongye-sun or @animeshsingh do you have possibility to review the PR?

Ark-kun
Ark-kun reviewed Jun 17, 2020
View reviewed changes
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated
@@ -24,6 +24,39 @@
# TODO(hongyes): make this path configurable as a environment variable
KFP_OUTPUT_PATH = '/tmp/kfp/output/'


def query_only(query, project_id, dataset_location='US', job_config=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the query function be used for this purpose? For example, it could work the same when dataset_id is None.

Ark-kun
Ark-kun reviewed Jun 17, 2020
View reviewed changes
components/gcp/bigquery/query/biqquery_query_to_table.yaml Outdated
for details.
default: ''
type: Dict
outputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have some output, so that more components can be chained to this one? E.g. the table where the results are written.

@Ark-kun
Copy link
Contributor

Ark-kun commented Jun 17, 2020

friendly ping @Ark-kun :)

Reviewed. Thanks for your patience.

I would love to merge your components.

@Ark-kun Ark-kun changed the title Update the GCP components Components - Added more GCP BigQuery components Jun 18, 2020
numerology
numerology reviewed Jun 18, 2020
View reviewed changes
Copy link

@numerology numerology left a comment

Choose a reason for hiding this comment

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

Thanks @NikeNano !

components/gcp/bigquery/query/to_gcs/component.yaml Outdated Show resolved Hide resolved
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated Show resolved Hide resolved
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated Show resolved Hide resolved
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated Show resolved Hide resolved
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated Show resolved Hide resolved
components/gcp/container/component_sdk/python/kfp_component/google/bigquery/_query.py Outdated Show resolved Hide resolved
@NikeNano
Copy link
Member Author

NikeNano commented Jun 20, 2020 •
edited
Loading

I have done some clean up now, thanks for the feedback @Ark-kun and @numerology. I will squash the commits. ~~ Do we want to add one read me for each or one combined? ~~ Added a readme to each of the folders as well.

@NikeNano NikeNano force-pushed the components_update branch from c5d693b to 04afdac Compare June 20, 2020 15:46
@Ark-kun
Copy link
Contributor

Ark-kun commented Jun 20, 2020

Thank you for your improvements!
/lgtm

@Ark-kun
Copy link
Contributor

Ark-kun commented Jun 23, 2020

/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

@Bobgy
Copy link
Contributor

Bobgy commented Jun 24, 2020

/retest

@NikeNano
Copy link
Member Author

/test kubeflow-pipeline-frontend-test

@NikeNano
Copy link
Member Author

/kubeflow-pipeline-backend-test

@Ark-kun
Copy link
Contributor

Ark-kun commented Jun 24, 2020

/retest

1 similar comment
@NikeNano
Copy link
Member Author

/retest

@NikeNano
Copy link
Member Author

/test kubeflow-pipeline-e2e-te

@k8s-ci-robot
Copy link
Contributor

@NikeNano: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test kubeflow-pipeline-frontend-test
  • /test kubeflow-pipeline-backend-test
  • /test kubeflow-pipeline-e2e-test
  • /test kubeflow-pipeline-sample-test
  • /test kubeflow-pipeline-integration-test
  • /test kubeflow-pipeline-mkp-test
  • /test kubeflow-pipeline-upgrade-test
  • /test kubeflow-pipelines-sdk-python35
  • /test kubeflow-pipelines-sdk-python36
  • /test kubeflow-pipelines-sdk-python37
  • /test kubeflow-pipelines-sdk-python38
  • /test kubeflow-pipelines-tfx-python36
  • /test kubeflow-pipelines-component-yaml
  • /test kubeflow-pipelines-components-gcp-python27
  • /test kubeflow-pipelines-components-gcp-python37
  • /test kubeflow-pipelines-component-containers
  • /test kubeflow-pipelines-backend-visualization
  • /test kubeflow-pipeline-multiuser-test

Use /test all to run the following jobs:

  • kubeflow-pipeline-frontend-test
  • kubeflow-pipeline-backend-test
  • kubeflow-pipeline-e2e-test
  • kubeflow-pipeline-sample-test
  • kubeflow-pipeline-upgrade-test
  • kubeflow-pipelines-sdk-python35
  • kubeflow-pipelines-sdk-python36
  • kubeflow-pipelines-sdk-python37
  • kubeflow-pipelines-sdk-python38
  • kubeflow-pipelines-tfx-python36
  • kubeflow-pipelines-component-yaml
  • kubeflow-pipelines-components-gcp-python27
  • kubeflow-pipelines-components-gcp-python37
  • kubeflow-pipelines-component-containers

In response to this:

/test kubeflow-pipeline-e2e-te

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.

@NikeNano
Copy link
Member Author

/test kubeflow-pipeline-e2e-test

@NikeNano
Copy link
Member Author

/retest

@k8s-ci-robot
Copy link
Contributor

@NikeNano: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test kubeflow-pipeline-frontend-test
  • /test kubeflow-pipeline-backend-test
  • /test kubeflow-pipeline-e2e-test
  • /test kubeflow-pipeline-sample-test
  • /test kubeflow-pipeline-integration-test
  • /test kubeflow-pipeline-mkp-test
  • /test kubeflow-pipeline-upgrade-test
  • /test kubeflow-pipelines-sdk-python35
  • /test kubeflow-pipelines-sdk-python36
  • /test kubeflow-pipelines-sdk-python37
  • /test kubeflow-pipelines-sdk-python38
  • /test kubeflow-pipelines-tfx-python36
  • /test kubeflow-pipelines-component-yaml
  • /test kubeflow-pipelines-components-gcp-python27
  • /test kubeflow-pipelines-components-gcp-python37
  • /test kubeflow-pipelines-component-containers
  • /test kubeflow-pipelines-backend-visualization
  • /test kubeflow-pipeline-multiuser-test

Use /test all to run the following jobs:

  • kubeflow-pipeline-frontend-test
  • kubeflow-pipeline-backend-test
  • kubeflow-pipeline-e2e-test
  • kubeflow-pipeline-sample-test
  • kubeflow-pipeline-upgrade-test
  • kubeflow-pipelines-sdk-python35
  • kubeflow-pipelines-sdk-python36
  • kubeflow-pipelines-sdk-python37
  • kubeflow-pipelines-sdk-python38
  • kubeflow-pipelines-tfx-python36
  • kubeflow-pipelines-component-yaml
  • kubeflow-pipelines-components-gcp-python27
  • kubeflow-pipelines-components-gcp-python37
  • kubeflow-pipelines-component-containers

In response to this:

/test kubeflow-pipeline-e2e-te

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.

@NikeNano
Copy link
Member Author

/test kubeflow-pipeline-e2e-test

@NikeNano
Copy link
Member Author

/test kubeflow-pipeline-upgrade-test

@k8s-ci-robot k8s-ci-robot merged commit c52a73e into kubeflow:master Jun 26, 2020
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

@numerology numerology numerology left review comments

@animeshsingh animeshsingh Awaiting requested review from animeshsingh

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

Assignees

@Ark-kun Ark-kun

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

Successfully merging this pull request may close these issues.
6 participants
@NikeNano @kubeflow-bot @Ark-kun @k8s-ci-robot @Bobgy @numerology