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

Include tags in the /measurements and /inputs APIs #361

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

kbenne
Copy link
Contributor

@kbenne kbenne commented Sep 24, 2021

ref #360

This implements a mechanism for capturing tags and returning them via the API. Additional work will be required to define high quality, standards compliant tags. There is an example of a tag file here https://github.com/ibpsa/project1-boptest/blob/issue360_tagging/testcases/testcase1/models/tags.json.

@kbenne kbenne requested a review from dhblum September 24, 2021 19:11
@dhblum
Copy link
Collaborator

dhblum commented Sep 27, 2021

Thanks @kbenne for this. I like having the tags.json and it's similarities to other .json data types we have, in terms of its inclusion in the test case fmu and then expression through the API. I have a couple of comments/questions though:

  1. While we had discussed leveraging the /measurements and /inputs api endpoints, and these indeed indicate how one could pass this information through to the user, one concern I'm realizing is that it then focuses on tags for I/O, but does not allow for (at least intuitively) tags for things like pieces of equipment, which are present in e.g. Haystack. I'm wondering if a new api endpoint then is needed to allow for this, and the return would be a tag report? Either something dedicated to equipment, or I'm thinking maybe more generally for all tags may be better.
  2. The example tags.json is I think based on Haystack. Just curious, what does m: mean?

@kbenne
Copy link
Contributor Author

kbenne commented Sep 27, 2021

  1. Indeed, BOPTEST API doesn't really expose the intermediate layers. To me, BOPTEST is just exposing the leaves of the tree. I'm not sure what the answer is. One thing that might be part of the solution is to eventually just support Haystack API to compliment the BOPTEST API. The Haystack API offering the full view of the model, the BOPTEST API sticking to I/O. In my mind they don't have to be exclusive, they can be complimentary and exist and be used simultaneuously. We could alternatively add endpoints to BOPTEST, but does that eventually start to just mirror Haystack, but less good?

  2. That one is easy. Yes the tags file is following haystack, and m: is a marker tag. https://project-haystack.org/doc/docHaystack/Kinds#marker Although it appears that Haystack is saying that json should use the full string "marker". I think they must have changed that in the last year or so.

Copy link
Collaborator

@dhblum dhblum left a comment

Choose a reason for hiding this comment

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

@kbenne I think eventually it would be good to at least be able to specify (in a test case) and get (as a user) a full haystack model, which if I understand correctly could be implemented as a json of tags. Does it require a full haystack API implementation to get the haystack model, or does one without the other sound ridiculous? A haystack API could indeed live alongside and be complementary to the current BOPTEST API, but with notable overlaps in functionality being PointWrite-/advance (though advance also advances the simulation) and HisRead-/results.

In the meantime for this PR, can you :

  • Address my inline comments and
  • Update design documentation to include tags.json in the test case directory spec (comment that it is optional right now), and also add a section and spec for tags.json as you've implemented similar to other json specs.
  • Update release notes with this edit

testcase.py Outdated
@@ -645,6 +645,7 @@ def _get_var_metadata(self, fmu, var_list, inputs=False):
maxi = None
var_metadata[var] = {'Unit':unit,
'Description':description,
'tags': self.tags_json.get(var),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to capitalize 'tags' like other meta data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certainly. good catch.

"point": "m:",
"sensor": "m:",
"temp": "m:",
"unit": "s:C"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unit should be K.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbenne I think eventually it would be good to at least be able to specify (in a test case) and get (as a user) a full haystack model, which if I understand correctly could be implemented as a json of tags. Does it require a full haystack API implementation to get the haystack model, or does one without the other sound ridiculous? A haystack API could indeed live alongside and be complementary to the current BOPTEST API, but with notable overlaps in functionality being PointWrite-/advance (though advance also advances the simulation) and HisRead-/results.

In the meantime for this PR, can you :

  • Address my inline comments and
  • Update design documentation to include tags.json in the test case directory spec (comment that it is optional right now), and also add a section and spec for tags.json as you've implemented similar to other json specs.
  • Update release notes with this edit

As far as I know, you don't have to implement all of the Haystack API. In fact the API specifies an /ops endpoint that returns a form of documentation for what endpoints the server is supporting. I think read and nav would be sufficient for model discovery.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then the use of /ops, read, and nav, along with implementing an example "full" model, I think could be a good next goal, at least in the direction of haystack support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For #361 (comment) the unit has been changed in 0868a08.

@dhblum
Copy link
Collaborator

dhblum commented Apr 6, 2022

Unit tests pass at commit 262c9f5.

Further commits after this store Haystack tags in the bestest_air case and prototype a workflow for their usage in creating the tags.json. In particular, they:

  • Update the bestest_air model to include signal exchange blocks that use Haystack tags as implemented in a fork of the Modelica Buildings Library at https://github.com/dhblum/modelica-buildings/tree/issue_boptest_360_haystack_tags. Such updates to Modelica Buildings Library via Modelica IBPSA Library still needs to be done, however, changes will be implemented in development branches of those libraries that use MSL 4.0, which BOPTEST needs to resolve through use of compilation tools other than JModelica (e.g. OpenModelica and Spawn).
  • Update the parser.py to parse the Haystack tags from the signal exchange blocks and create the tags.json.
  • Update the bestest_air wrapped fmu as an example of making the Haystack tags available through BOPTEST.
  • Update the Design Requirements and Guide to include a new section on how tags are stored in Modelica model with proposed updates to signal exchange block and then used to create the tags.json.
  • Will not pass unit tests because the utilized Modelica Buildings Library for compilation of test cases during tests needs to be changed to the forked implementation and/or the parser.py in this branch needs to be updated to compile cases that do not use the implementation of Haystack tags in the signal exchange blocks.

The output of /measurements using the bestest_air test case in this branch now gives for example:

Screen Shot 2022-04-06 at 5 25 12 PM

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.

2 participants