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

Remote libraries (and bulk library loading) #213

Merged
merged 19 commits into from
Mar 1, 2023
Merged

Conversation

gtfierro
Copy link
Collaborator

Screenshot 2023-01-25 at 6 14 00 PM

I've created a simple buildingmotif CLI tool for loading libraries into a buildingmotif instance from a libraries.yml file. POC is in the attached screenshot. Address #207 and is one answer to #133 (unless I'm misinterpreting that issue?)

I will definitely need to add some documentation on the libraries.yml format and how the CLI tool works, but what do you all think of the solution so far?

@TShapinsky @MatthewSteen @haneslinger

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Base: 73.54% // Head: 72.71% // Decreases project coverage by -0.83% ⚠️

Coverage data is based on head (52aa50d) compared to base (5dba327).
Patch coverage: 16.66% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #213      +/-   ##
===========================================
- Coverage    73.54%   72.71%   -0.83%     
===========================================
  Files           31       31              
  Lines         2026     2056      +30     
===========================================
+ Hits          1490     1495       +5     
- Misses         536      561      +25     
Impacted Files Coverage Δ
buildingmotif/dataclasses/library.py 82.25% <16.66%> (-9.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@MatthewSteen MatthewSteen left a comment

Choose a reason for hiding this comment

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

Looking good. A few questions and comments.

  1. What's the benefit of using a YML file vs. passing local dirs or remote repos as CLI arguments?
  2. Why were the changes to docs/tutorials/tutorial3_model.ttl required?
  3. Do we need to track output.ttl, which changes every time the 223PExample.ipynb is run (e.g. when integration test are run)?

docs/tutorials/tutorial3_model.ttl Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@MatthewSteen
Copy link
Member

Should the base branch = develop now?

@gtfierro gtfierro mentioned this pull request Feb 21, 2023
1 task
@gtfierro gtfierro changed the base branch from main to develop February 21, 2023 21:12
@gtfierro gtfierro marked this pull request as ready for review February 21, 2023 21:14
@gtfierro
Copy link
Collaborator Author

Thanks for the review, @MatthewSteen ! Here's some answers:

What's the benefit of using a YML file vs. passing local dirs or remote repos as CLI arguments?
Basically convenience -- I can give you a libraries.yml file as a quick way of bootstrapping your installation. I've also added the ability to load local directories and local/remote ontologies via the CLI tool. Adding Git repos through the CLI tool is trickier because there are multiple arguments that need to be provided

Why were the changes to docs/tutorials/tutorial3_model.ttl required?

Not needed! Removed

Do we need to track output.ttl, which changes every time the 223PExample.ipynb is run (e.g. when integration test are run)?

Nope :). I've removed it and also added a gitignore rule

@gtfierro gtfierro requested review from MatthewSteen, haneslinger and TShapinsky and removed request for TShapinsky and haneslinger February 21, 2023 21:23
@haneslinger
Copy link
Collaborator

It's looking pretty good. can we get some docs on the format this yaml should be in maybe?

@gtfierro
Copy link
Collaborator Author

It's looking pretty good. can we get some docs on the format this yaml should be in maybe?

Done!

Copy link
Member

@TShapinsky TShapinsky left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks Gabe!

@gtfierro gtfierro merged commit fe6972b into develop Mar 1, 2023
@gtfierro gtfierro deleted the gtf-remote-libraries branch March 1, 2023 18:18
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.

5 participants