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

Create an Interface for programatically creating shapes #165

Merged
merged 13 commits into from
May 8, 2023

Conversation

TShapinsky
Copy link
Member

@TShapinsky TShapinsky commented Nov 10, 2022

Create initial base classes for shape builders.

Demo:
https://github.com/NREL/BuildingMOTIF/blob/shape_builder/notebooks/Shape_Builder.ipynb

TODO:

  • add missing functionality
  • create tests
  • Demo easy manifest creation
  • Add doc strings

closes #116

@TShapinsky TShapinsky changed the title Create an Interface for programatically creating shapes [WIP] Create an Interface for programatically creating shapes Nov 10, 2022
Copy link
Collaborator

@gtfierro gtfierro left a comment

Choose a reason for hiding this comment

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

Looking really great! Couple small changes and fixes

buildingmotif/shape_builder/shape.py Show resolved Hide resolved
buildingmotif/shape_builder/shape.py Outdated Show resolved Hide resolved
buildingmotif/shape_builder/shape.py Show resolved Hide resolved
buildingmotif/shape_builder/shape.py Outdated Show resolved Hide resolved
@TShapinsky TShapinsky changed the title [WIP] Create an Interface for programatically creating shapes Create an Interface for programatically creating shapes Mar 10, 2023
@TShapinsky
Copy link
Member Author

Once the API structure seems OK I'll move onto the additional steps

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (25a234c) 73.40% compared to head (280ee59) 73.40%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #165   +/-   ##
========================================
  Coverage    73.40%   73.40%           
========================================
  Files           31       31           
  Lines         2012     2012           
========================================
  Hits          1477     1477           
  Misses         535      535           

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TShapinsky
Copy link
Member Author

@gtfierro Do you have any idea how this PR caused these failures?

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023 via email

@TShapinsky
Copy link
Member Author

TShapinsky commented May 5, 2023

My guess is it is related to the version bumps on rdflib and pyshacl. I've pushed a couple commitments which I hoped would solve the issues, but it looks like there's a couple outstanding ones. I should be able to continue work on it this weekend

I thought the same thing too. The only thing that wigs me out is that it should be installing dependencies from the poetry.lock not resolving them. And I didn't update the lock

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023

Regardless of why the deps changed, I think I've almost fixed all of the issues. The last bit I think is due to the prefix declarations for SHACL rules not being handled correctly in the earlier version of Brick that is currently committed to the repo. This is fixed in a more recent version of Brick, but updating to the recent version would also require handling imports correctly in BuildingMOTIF. I've pushed a "quickfix" but eventually we should update Brick and handle ontology imports

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023

@haneslinger it looks like the last failing test has to do with the Flask API server in tests/unit/api/test_library.py:73

2023-05-05T17:54:13.1421712Z         # Act
2023-05-05T17:54:13.1421949Z         results = client.get("/libraries/shapes")
2023-05-05T17:54:13.1422170Z     
2023-05-05T17:54:13.1422329Z         # Assert
2023-05-05T17:54:13.1422546Z >       assert results.status_code == 200
2023-05-05T17:54:13.1422770Z E       assert 500 == 200
2023-05-05T17:54:13.1423066Z E        +  where 500 = <WrapperTestResponse streamed [500 INTERNAL SERVER ERROR]>.status_code

I can't find anything in the logs that suggests what is going wrong. Any ideas? Otherwise I can start adding some logging statements. Can't get it to reproduce locally

@TShapinsky
Copy link
Member Author

@haneslinger it looks like the last failing test has to do with the Flask API server in tests/unit/api/test_library.py:73

2023-05-05T17:54:13.1421712Z         # Act
2023-05-05T17:54:13.1421949Z         results = client.get("/libraries/shapes")
2023-05-05T17:54:13.1422170Z     
2023-05-05T17:54:13.1422329Z         # Assert
2023-05-05T17:54:13.1422546Z >       assert results.status_code == 200
2023-05-05T17:54:13.1422770Z E       assert 500 == 200
2023-05-05T17:54:13.1423066Z E        +  where 500 = <WrapperTestResponse streamed [500 INTERNAL SERVER ERROR]>.status_code

I can't find anything in the logs that suggests what is going wrong. Any ideas? Otherwise I can start adding some logging statements. Can't get it to reproduce locally

I don't have any ideas about that, I'm digging into some stuff locally

@TShapinsky
Copy link
Member Author

@gtfierro I was able to reproduce by doing poetry install --all-extras since that's what the ci does

@TShapinsky
Copy link
Member Author

It is an "Unknown namespace prefix : sh" error when it attempts to run the shape query. Any ideas? @gtfierro

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023

It is an "Unknown namespace prefix : sh" error when it attempts to run the shape query. Any ideas? @gtfierro

Have you pulled latest from this branch? I think I've fixed that so its just the Flask error

@TShapinsky
Copy link
Member Author

It is an "Unknown namespace prefix : sh" error when it attempts to run the shape query. Any ideas? @gtfierro

Have you pulled latest from this branch? I think I've fixed that so its just the Flask error

Yeah, that is the flask error I believe, if you just print the response text in the test that is the string you get with the 500 error.

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023

Were you able to reproduce the Flask error locally? I'm not able to even with installing with --all-extras. Or are you getting the response text some other way? Doesn't seem to be in the error trace online

@gtfierro
Copy link
Collaborator

gtfierro commented May 5, 2023

I think I might have found the bug! We were missing prefixes in the SPARQL query; later versions of RDFlib/pyshacl seem to care more about having prefixes declared rather than inferring them from other graphs

Copy link
Collaborator

@gtfierro gtfierro 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 good with these changes! Glad we figured out the tests at last

@TShapinsky TShapinsky merged commit 0a17328 into develop May 8, 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.

Create abstractions for creating common shacl shapes/constraints
4 participants