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

optionally provide sample-map-file instead of sample-map-table #6872

Merged
merged 6 commits into from
Oct 21, 2020

Conversation

ericsong
Copy link

@ericsong ericsong commented Oct 8, 2020

Changes

  • --cohort-sample-file can be used instead of --cohort-sample-table to provide the (sample_id,sample_name) map
  • The Avro reader was changed to set the BQ client's parent to be the current project instead of the destination table's project. This was needed for AoU since our notebook users do not have BQ jobs permissions on the microarray dataset.

@ericsong
Copy link
Author

@meganshand do you have any other ideas for how I can fix the test failure?

@meganshand
Copy link
Contributor

The issue with the tests is that this PR is from a fork so the tests don't have access to the permissions we need for BigQuery in the variantstore WDL tests. I need to fix this for the future (by making sure those tests don't run when the PR is from a fork), but I don't think that test failure should block this PR.

@calbach
Copy link

calbach commented Oct 16, 2020

What's the protocol for getting this PR merged - @meganshand are you the right reviewer?

@@ -50,7 +51,7 @@ public StorageAPIAvroReader(final TableReference tableRef, final String rowRestr
try {
this.client = BigQueryStorageClient.create();

final String parent = String.format("projects/%s", tableRef.tableProject);
final String parent = String.format("projects/%s", BigQueryOptions.getDefaultInstance().getProjectId());
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two implication here -- you need permission, but also this is where the cost of using the storage API will be attributed. This was set to the destination table project since (a) the user just created that table so they should have permission and (b) the cost for the temporary storage of that table and the cost to use the read API against it should go to to the same place.

If we need the extra flexibility to provide an independent project here, rather than the default I would make another argument like "read-api-project" or something that can be passed in to override the default of the table project

@ericsong
Copy link
Author

I think it should be good for another pass @kcibul @meganshand

Copy link
Contributor

@kcibul kcibul left a comment

Choose a reason for hiding this comment

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

lgtm

@kcibul kcibul merged commit 0360115 into broadinstitute:ah_var_store Oct 21, 2020
kcibul pushed a commit that referenced this pull request Jan 29, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
kcibul pushed a commit that referenced this pull request Feb 1, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
Marianie-Simeon pushed a commit that referenced this pull request Feb 16, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
kcibul pushed a commit that referenced this pull request Mar 9, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
mmorgantaylor pushed a commit that referenced this pull request Apr 6, 2021
* optionally provide sample-map-file instead of sample-map-table

* fix variantstore test

* trying to fix test

* pass in read project id

* empty string -> null

* make read-project-id optional

Co-authored-by: Megan Shand <mshand@broadinstitute.org>
This was referenced Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants