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

Update sheet2linkml so that it can be run from the root directory of this package #96

Merged
merged 24 commits into from
Sep 15, 2021

Conversation

gaurav
Copy link
Collaborator

@gaurav gaurav commented Sep 7, 2021

This PR updates sheet2linkml so that it can be run from the root directory of this package. To do this:

  1. I've updated sheet2linkml so that it can write the model to an output filename specified by a command line argument.
  2. I've updated the Makefile so that regen-google-sheets (now renamed to generate-model) now includes the Google Sheet ID and no longer changes directory when running.
  3. I've updated the READMEs so that Google Drive authentication files are created in the project root directory.
  4. I've reactivated the gh-deploy target in the Makefile so we can once again publish these changes to GitHub Pages using mike.
  5. I moved generators/google-sheets/sheet2linkml to vendors/sheet2linkml/sheet2linkml.

Along with these changes, I've also generated the model and all of its artifacts.

Closes #95, closes #84.

Copy link
Contributor

@sujaypatil96 sujaypatil96 left a comment

Choose a reason for hiding this comment

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

Left a few fairly trivial comments on this PR. Feel free to address them in this PR or separate ones, but for the most part this PR is good to go.🚀

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
vendor/sheet2linkml/sheet2linkml/cli.py Show resolved Hide resolved
vendor/sheet2linkml/sheet2linkml/cli.py Show resolved Hide resolved
@sujaypatil96 sujaypatil96 linked an issue Sep 7, 2021 that may be closed by this pull request
2 tasks
@turbomam
Copy link
Member

turbomam commented Sep 9, 2021

Do the rest of you get something like this when you authenticate for the Google Sheets API (in Chrome?)

Google hasn’t verified this app
The app is requesting access to sensitive info in your Google Account. Until the developer ([email protected]) verifies this app with Google, you shouldn't use it.
If you’re the developer, submit a verification request to remove this screen. Learn more
Hide Advanced
Continue only if you understand the risks and trust the developer ([email protected]).
Go to crdch_model (unsafe)

I figure we should either eliminate this warning or at least warn "other users" that they may see soemthing liek this and explain why.

@turbomam
Copy link
Member

turbomam commented Sep 9, 2021

What did we decide to do about warnings like

Thu Sep 9 10:07:22 2021 [entity.py] WARNING: In field 'CRDC-H.BodySite.site', found duplicate permissible value text: Blood was previously assigned to PermissibleValue(text='Blood', description=None, meaning=None, is_a=None, mixins=[], extensions={}, annotations={}, alt_descriptions={}, deprecated=None, todos=[], notes=[], comments=[], examples=[], in_subset=[], from_schema=None, imported_from=None, see_also=[], deprecated_element_has_exact_replacement=None, deprecated_element_has_possible_replacement=None), but will now be replaced with {'text': 'Blood', 'description': 'Blood'}

@turbomam
Copy link
Member

turbomam commented Sep 9, 2021

I'm tempted to suggest modifying the README to include an estimate of how long to takes to generate the model form the Google Sheet, although there's nothing analogous for the other steps in the Makefile.

Copy link
Member

@turbomam turbomam left a comment

Choose a reason for hiding this comment

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

This modified code for generating the model from the Google Sheet looks good and runs in my environment.

I left some comments but wasn't in review mode at the time.

  • I marked @sujaypatil96's comment about using the vendor directory as complete.
  • I agree with his suggestion for a short -o parameter for specifying the model generation's output file, but that seems minor.
  • I defer to @sujaypatil96's judgement about logging practices.
  • I think the main README is good but the Google Sheet model generation README could use some clarification or updating. I'll take a stab at that now. That could include a warning that web browsers may consider the process of authenticating the Google Sheets API unsafe, if other people are getting messages like the one I pasted above.
  • I get a lot of warnings during the model generation. I think @gaurav and I have spoken about that before and that it might require making a request of the people who maintain the Google Sheet.

Base automatically changed from issue-69-pypi to main September 9, 2021 18:14
@gaurav
Copy link
Collaborator Author

gaurav commented Sep 9, 2021

What did we decide to do about warnings like

Thu Sep 9 10:07:22 2021 [entity.py] WARNING: In field 'CRDC-H.BodySite.site', found duplicate permissible value text: Blood was previously assigned to PermissibleValue(text='Blood', description=None, meaning=None, is_a=None, mixins=[], extensions={}, annotations={}, alt_descriptions={}, deprecated=None, todos=[], notes=[], comments=[], examples=[], in_subset=[], from_schema=None, imported_from=None, see_also=[], deprecated_element_has_exact_replacement=None, deprecated_element_has_possible_replacement=None), but will now be replaced with {'text': 'Blood', 'description': 'Blood'}

Most of the duplicate permissible value warnings should go away once the CCDH Terminology Service stops giving us duplicate PVs (cancerDHC/ccdh-terminology-service#27), and then anything that remains will be an error in the Google Sheet. I think this warning is fine for now.

@gaurav
Copy link
Collaborator Author

gaurav commented Sep 10, 2021

Do the rest of you get something like this when you authenticate for the Google Sheets API (in Chrome?)

Google hasn’t verified this app
The app is requesting access to sensitive info in your Google Account. Until the developer ([email protected]) verifies this app with Google, you shouldn't use it.
If you’re the developer, submit a verification request to remove this screen. Learn more
Hide Advanced
Continue only if you understand the risks and trust the developer ([email protected]).
Go to crdch_model (unsafe)

I figure we should either eliminate this warning or at least warn "other users" that they may see soemthing liek this and explain why.

I don't see this. On the "OAuth consent screen" tab on your GCI APIs and Services page, could you please make sure that your app has a publishing status of "Testing" and that your e-mail address has been added to the list of Test Users? I think that's the way to suppress this warning.

@turbomam
Copy link
Member

Do the rest of you get something like this when you authenticate for the Google Sheets API (in Chrome?)

Google hasn’t verified this app
...

I don't see this. On the "OAuth consent screen" tab on your GCI APIs and Services page, could you please make sure that your app has a publishing status of "Testing" and that your e-mail address has been added to the list of Test Users? I think that's the way to suppress this warning.

Thanks. I think I've tried various OAuth consent configurations over the weeks. My latest workflow can be found in #98
I didn't get this warning after using that approach. Sorry, I should have passed that on.

I'll try these suggestions, too.

Copy link
Contributor

@sujaypatil96 sujaypatil96 left a comment

Choose a reason for hiding this comment

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

Great work on this PR @gaurav. I think it's ready to be merged.🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants