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

Changed folder structure for ontologies, review based changes #4

Merged
merged 5 commits into from
Feb 23, 2022

Conversation

Piyush13y
Copy link
Collaborator

@Piyush13y Piyush13y commented Feb 19, 2022

This PR fixes Issue#1

Description of changes

  • Changed structure for ontologies from the previous PR based on reviews and deleted redundant files.

Has the following structure now:

- forte_medical 
- ftx 
  |_ medical
      |_ __init__.py 
      |_ clinical.py
  • Updated example to work with correct namespaces now.
  • Modified setup.py to use find_namespace_packages() now
  • Added a .gitignore file

@Piyush13y Piyush13y changed the title Added ftx for ontologies, review based changes Changed folder structure for ontologies, review based changes Feb 19, 2022
@hunterhector
Copy link
Member

hunterhector commented Feb 19, 2022

Thanks for the fix. One thing missing is how the setup.py work (https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages), we need to let it know that we use certain namespaces.

Btw, to enforce this convention and make everything consistent in the future, we probably should also adopt the same convention in Forte main repo (this should be an issue on Forte):

this includes several small steps:

  1. update https://github.com/asyml/forte/blob/master/forte/ontology_specs/medical.json which in turn that package path, to make them not in the "ftx" package directly. Need to also update the usage of this (in the examples you try before, but we should search for the usage more carefully).
  2. after generating the code, we remove the init file here: https://github.com/asyml/forte/tree/master/ftx
  3. update setup.py according to this: https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages
    • packages=find_namespace_packages(include=['ftx.*'])

The reason is that namespace packaging requires us to use the same convention everywhere, otherwise weird things would happen during import.

@Piyush13y Piyush13y requested review from hunterhector and removed request for tanyuqian February 23, 2022 05:43
Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

Consider commit a .gitignore file to skip some files, you can use this sample: https://github.com/asyml/forte/blob/master/.gitignore

@Piyush13y Piyush13y merged commit 3830871 into master Feb 23, 2022
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.

mimic-iii pipeline
2 participants