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

Implement clean up strategies #128

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sujaypatil96
Copy link
Contributor

This PR seeks to implement some of the repo clean up strategies discussed in issue #105. The major change in this PR is the integration of the sheet2linkml package as a dependency in the project, and the consequent removal of the vendor directory which previously housed sheet2linkml.

@turbomam
Copy link
Member

turbomam commented Oct 6, 2021

I'm OK with the examples-for-discussionexamples directory name change, but I don't remember any discussions bout it. Didn't somebody from outside of the tools group create that directory or contribute to it? Is everyone involved OK with the name change?

@turbomam
Copy link
Member

turbomam commented Oct 6, 2021

why are we renaming model/linkml_model? Is this something that we're going to start doing for most linkml projects that BBOPers contribute to?

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.

  • I'm OK with the examples-for-discussionexamples directory name change, but I don't remember any discussions bout it. Didn't somebody from outside of the tools group create that directory or contribute to it? Is everyone involved OK with the name change?
  • why are we renaming model/linkml_model? Is this something that we're going to start doing for most linkml projects that BBOPers contribute to?

.github/workflows/tox.yml Outdated Show resolved Hide resolved
generate-model: install install-dev
CDM_GOOGLE_SHEET_ID=$(CDM_GOOGLE_SHEET_ID) $(RUN) python vendor/sheet2linkml/sheet2linkml.py --output model/schema/crdch_model.yaml
CDM_GOOGLE_SHEET_ID=$(CDM_GOOGLE_SHEET_ID) $(RUN) sheet2linkml --output linkml_model/schema/crdch_model.yaml --logging-config ./logging.ini
Copy link
Member

Choose a reason for hiding this comment

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

I haven't run this within this repo, but I have run it directly in cancerDHC/sheet2linkml, so I trust it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the output file should be --output model/schema/crdch_model.yaml, unless we're moving the directory we store crdch_model.yaml in?

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.

BTW I'm glad you're taking the initiative to cleanup, @sujaypatil96

@sujaypatil96
Copy link
Contributor Author

sujaypatil96 commented Oct 6, 2021

I'm OK with the examples-for-discussion → examples directory name change, but I don't remember any discussions bout it. Didn't somebody from outside of the tools group create that directory or contribute to it? Is everyone involved OK with the name change?

Hm, I think I did bring it up during one of our meetings, but there wasn't a particularly long discussion about it. That makes sense, I CC the people involved and check if they're okay with the name change.

Edit: There's a discussion thread for the above here.

why are we renaming model/→ linkml_model? Is this something that we're going to start doing for most linkml projects that BBOPers contribute to?

I though it'd be a good idea to rename it to linkml_model since in the same way that we have the crdch_model folder in the repo, which is pretty self explanatory as to what it houses, model was too generic to be semantically useful. Which is why I recommended a change to linkml_model so we know its contents.

@turbomam
Copy link
Member

turbomam commented Oct 7, 2021

Edit: There's a discussion thread for the above here.

Thanks. We should check with @gaurav and make sure that we're not confusing the examples-for-discussion with examples that come from the example-data repo

@gaurav
Copy link
Collaborator

gaurav commented Oct 14, 2021

Might overlap with #125

Copy link
Collaborator

@gaurav gaurav left a comment

Choose a reason for hiding this comment

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

I would stick with model/ instead of linkml_model/ as the directory for the model -- partially because I think of linkml-model as being the only true "linkml model", but mostly because I think we should follow linkml-model-template in referring to this as the "model" directory. I think the best name for this directory might be "src", but if so, that change should be made in linkml-model-template before it propagates down to us, so that all LinkML models can share a similar directory structure, making it easy for LinkML users to jump between them.

generate-model: install install-dev
CDM_GOOGLE_SHEET_ID=$(CDM_GOOGLE_SHEET_ID) $(RUN) python vendor/sheet2linkml/sheet2linkml.py --output model/schema/crdch_model.yaml
CDM_GOOGLE_SHEET_ID=$(CDM_GOOGLE_SHEET_ID) $(RUN) sheet2linkml --output linkml_model/schema/crdch_model.yaml --logging-config ./logging.ini
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the output file should be --output model/schema/crdch_model.yaml, unless we're moving the directory we store crdch_model.yaml in?

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.

I don't have 3.7, so tox skipped that. 3.8 and 3.9 passed.

Maybe somebody can teach me how to install an old version of Python in a virtual environment for more thorough tox testing. I haven't been in the habit of installing old Python versions into my global environment.

Also, why does the 3.8 test bundle work on my computer but not within the GH action?

@gaurav gaurav marked this pull request as draft December 16, 2021 20:08
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.

3 participants