Add pydantic model

[Speters1086]

This PR introduces a Pydantic data model built off of the JSON schema. This is useful for making use of the schema in Python.

Also introduced in this PR is:

• a poetry environment for managing python dependencies.
• a test to test the pydantic data model on the demo data in the repo
• a CI build that builds, lints, and tests the pydantic data model

one modification was made to the JSON Schema to remove special characters from a description of a single field. These characters were causing errors when generating the pydantic model using the code generator.

[stephenholleran]

Hi @Speters1086,

This looks great! Thanks for contributing. I'm still getting my head around all the files but from the outset it looks good with your good documentation.

I went to use the Notebook but got an error.

This looks like that the 'src' folder is at the same level as the 'tools' folder and not a child of where the Notebook is located.

@abohara what do you think of this? You have more coding experience than me so would be great to get your opinion.

[Speters1086]

@stephenholleran, this was a bit of a pain on my end. In the end my setting.json in .vscode looked like this to and everything just worked.

{
    "terminal.integrated.env.osx": {
      "PYTHONPATH": "${workspaceFolder}/.",
    },
    "terminal.integrated.env.linux": {
      "PYTHONPATH": "${workspaceFolder}/.",
    },
    "terminal.integrated.env.windows": {
      "PYTHONPATH": "${workspaceFolder}/.",
    },
    "python.envFile": "${workspaceFolder}/.env",
    "jupyter.notebookFileRoot": "${workspaceFolder}/.",
    "python.autoComplete.extraPaths": [
        "${workspaceFolder}/."
    ],
    "python.pythonPath": ".venv/Scripts/python.exe"
  }

[jonssonchristian]

Resolves #244

[jonssonchristian]

I think this is a great contribution that will provide great ergonomics for working with the data model in a Python environment.

[stephenholleran]

Thanks @jonssonchristian for taking a look. Some of your comments are way over my understanding so appreciate someone else taking a look.

[Speters1086]

Thanks @jonssonchristian for the review. I am on holiday and will get back to this PR when I am back.

@stephenholleran how close is the floating lidar branch to being merged into dev/main, if not done already?

I will hold this PR until that branch is committed to dev so I can update the pydantic model with any changes.

With this PR the test data in the repo is used to test the model. Is the test data exhaustive? I had thought about adding test coverage to give an idea about this. @jonssonchristian thoughts?

[jonssonchristian]

I'm just about to leave for holidays and will return the week after next. I'll have a look and share thoughts when I get back.

[stephenholleran]

Hi @Speters1086,

@stephenholleran how close is the floating lidar branch to being merged into dev/main, if not done already?

The floating lidar file format #271 is pretty much done. Just waiting on at least another person to review it. Aim is to merge it into the dev branch on the call in 2 weeks, 1st May.

There is another branch #282 which contains additional sensor_types and measurement_types falling out from the floating lidar discussions. This PR is up to date with all the decisions currently made but there are a few more items to discuss. Things about wave spectra or wave heights/periods that may or may not required any changes. These are the last few points in today's call that we didn't get to #268 (comment)
These could take a few weeks yet.

Thanks again!

[abohara]

Hi @stephenholleran , @Speters1086 , @jonssonchristian

Thanks for all your work on this. I realize I am late to this conversation, so please just take these as suggestions. I very much value having Pydantic models that can be leveraged in python code for type checking. However, I do have some arguments against including it in the current form (i.e. in the root folder ) prominently in this repository .

1. This repository is primarily a specification expressed via JsonSchema. We have a JSON example that we keep in sync as well as postgres schema and ipython examples. For every change to the core JSONSchema , we need to update these additional documents to keep them in sync. So, Pydantic models add another long term maintenance responsibilities to any future updates ( though I do realize it is auto generated by github actions ). We currently have the documentation generation and the form generator that were added in similar vein but we have add issues troubleshooting and keeping them working ( we probably should remove the form generation ). So, the long term maintenance of supporting Pydantic models I think is a concern , especially given relatively recent changes to v2 that did break backward compatibility ( I believe ) and a likely change to v3 in the future .

2. Since, the Pydantic model code is auto generated, would updating the 'Readme' to inform how a user could leverage the 'JSONSchema' to generate & Pydantic models , be a lighterweight but sufficient alternative to help a user generate Pydantic code ?

3. If we do include it , could it put it within the tools section like we do for the postgres schema generation ? This way we would not detract from the core specification available as the JSON schema ? It would also serve as a useful example if another use wanted to add a similar Typescript model ?

4. Or maybe even in its own repository with a prominent link to it ?

I think the Pydantic models are a great addition , not to mention they are much more readable compared to a JSONSchema. I am mostly concerned about the long term maintainability and the ergonomics of including it as a core part of the repo. Again apologies for jumping in late here, but @stephenholleran hope these help somewhat with the discussion.

[Speters1086]

@abohara, you make a valid point. I struggled with where to put this a bit, and hadn't thought to put it in the tools folder. I think this makes more sense, so I will address the other PR comments and then refactor the structure. I will also update the README.md to include a section explaining the Pydantic model. If we captured code coverage, this would also be displayed in the section, along with the build status for the model tests.

I also agree that there is an overhead to maintaining the Pydantic model, however, my view is that the benefits of ease of use test testability outweigh the overhead cost, but happy to take other views. You are correct that a code generator is used to create the model, but it is not automatically done. The current process is described in src/pydantic_model_creation_notes.md.

The other aspect of this is how settled this standard is. From the change history, it looks like the pace of change is relatively low, and thus, the overhead will be minor.

[Speters1086]

I have made some changes based on the PR comments and would like to ask for additional review.

I have rebased my changes on the latest version of the dev branch.

@jonssonchristian, I have addressed most of your PR comments. Please have a look and let me know if you still have any questions.

@abohara, I have restructured the repo slightly to pull out the PostgreSQL and Pydantic Models into their own folder and changed the tools directory to examples. This makes sense as the other tools, such as the app were already in their own directory. I have also updated the README to explain the difference between Tools and Examples.

I have also updated the example notebooks to work with the poetry environment provided and fixed some minor bugs so they will run though to completion.

@stephenholleran the 2023 Workshop - Floating Lidar Demo - Daniel Nuno.ipynb is dependent on data that is not in the repo. Should this example be removed?

[jonssonchristian]

With this PR the test data in the repo is used to test the model. Is the test data exhaustive? I had thought about adding test coverage to give an idea about this. @jonssonchristian thoughts?

I think it would be useful to add test coverage.

All that should be needed is to add pytest-cov to the external package requirements and then to include the below (or similar) in the pyproject.toml file or as CLI arguments in the pytest step. I am however not sure if there is a poetry-specific way of doing it.

[tool.pytest.ini_options]
addopts = [
    "--cov=wra_data_model_pydantic",
    "--cov-report=html",
    "--cov-report=xml",
    "--cov-report=term-missing",
]

[jonssonchristian]

Hi @Speters1086,

Thank you for the clarifications and updates. It looks good to me. In the following are some general comments and ideas, which may or may not be relevant, and some are certainly outside the scope of this PR and more as possible future developments.

• When we represent JSON arrays as sets, are we sure that users never rely on the order of the elements? I imagine the order will not be significant, more than perhaps for convenience when displaying the items. If someone parses JSON data into the Pydantic model, edits some data, and writes back to JSON, all the arrays may have random order. If there are duplicate items, the duplicates will simply be dropped without raising an error or warning that there is an issue with the input data. The JSON Schema arrays have "uniqueItems": true, so validation would fail if the items were not all unique.

• I agree that the Pydantic model increases maintenance effort, but should be worthwhile given how many users are working in a Python environment and how much it can benefit them. From my perspective, the most important thing would be to add automatic checks to ensure that the JSON Schema and Pydantic model fall out of sync. The tests on the examples already cover that, and more checks can probably be added in due course. It is very easy to miss something when aligning different content (even making sure all examples are updated correctly when updating the schema), and built-in checks can be really helpful.

• I think it would be useful to publish the Pydantic model in a pip package so that users can install it very easily into their Python environment and control the version dependency, rather than fetching the files from GitHub.

• There may be some advantages of separating all the Python-related content into a separate directory, also the pyproject.toml, poetry related files and tests. That might make it easier to navigate what files are related to what. The EYA DEF repo is currently structured like that, where eya_def_tools is the Python library.

[Speters1086]

@jonssonchristian, Thank you for your help with this and sorry it has taken me so long to get back to this. Your first point above is a very good one and something I had meant to come back to. I have updated the Pydantic model to no longer use sets and to validate the uniqueness of lists instead. As for the other points, I agree with them all, but think they should be addressed in separate PRs.

@stephenholleran, I know there is an upcoming release of the schema. I have rebased this branch of dev so it should be up to date. I am not sure about the protocol, as this is my first contribution here. Should I merge this into the dev branch?

[stephenholleran]

Hi @Speters1086,

Thanks for your work on this.

Sorry I haven't had much time to spend on this. I have been focused on getting the additional floating lidar parameters over the line for the next release. I had hoped to have completed that last Friday but there are some reviews that I still need to deal with. We decided on the call last Thursday to get the release out and then focus on this Pydantic work you have been doing and do another minor release (if it needs a release as it is not impacting the Schema). With WindEurope this week and WESC next week it is going to be challenging.

I can merge it once I get my head around it. I would like to go back to Amit's comments and understand those better too as it will most likely be me that will be trying to keep it updated.

Cheers,

[CLAassistant]

All committers have signed the CLA.

[olivia-bentley]

Hi @Speters1086, I've been reviewing this branch on Stephen's behalf and made a few small changes in the last commit. Please take a look and let me know if you have any concerns or would like to discuss any of the changes. Thanks for all your work on this I think incorporating pydantic here is a very valuable addition.

[Speters1086]

@olivia-bentley, sorry, this has gone off the boil for me. I can get back to this in a bit. it looks like this branch is quite a bit behind the dev branch, so it probably needs to be updated before this can be reviewed again. I can try to prioritise this in the next couple of weeks.

The main question is, are we happy with the structure the way it is or should the repo be refactored? I am not pressed about this, but it will mean changing some build steps.

[olivia-bentley]

@Speters1086 myself and Stephen would be happy to take on the work to bring it up to date with dev and think about the structure. It will be great to be able to bring the work into the library with credit to you for adding this.

[Speters1086]

@olivia-bentley, no worries... happy for you to get this over the line.