xpublish-community / xpublish

Publish Xarray Datasets via a REST API.
https://xpublish.readthedocs.io
Apache License 2.0
168 stars 23 forks source link

Pydantic 2 support #214

Closed abkfenris closed 1 year ago

abkfenris commented 1 year ago
          Ruh-row, it looks like we've been bitten by a Pydantic 2 change.

Originally posted by @abkfenris in https://github.com/xpublish-community/xpublish/issues/213#issuecomment-1635656104

It appears that we may need some tweaks for Pydantic 2 support.

https://github.com/xpublish-community/xpublish/actions/runs/5553093015/jobs/10141236050?pr=213#step:6:114

abkfenris commented 1 year ago

I think it may be that we need to annotate any fields that we've overridden, like name on plugins.

https://github.com/xpublish-community/xpublish/blob/b425bc1fb5125e79d747827a2a7d73910fd2d9b4/tests/test_rest_api.py#L66-L76

becomes

 class AirtempPlugin(Plugin): 
     name: str = 'airtemp' 

     @hookimpl 
     def get_dataset(self, dataset_id: str): 
         if dataset_id == 'airtemp': 
             return airtemp_ds 

     @hookimpl 
     def get_datasets(self): 
         return ['airtemp'] 
xaviernogueira commented 1 year ago

I just noticed this! I'm on it.

Will have it done today or tomorrow at the latest. Just had to update it on catalog-to-xpublish

abkfenris commented 1 year ago

Awesome, thank you. I think it's adding the annotations, and then probably adding a note to the docs that the annotations need to be in all Plugin subclasses.

I think the existing plugins may need an update for compatability too.

xaviernogueira commented 1 year ago

Just made a pull request #215. Mind checking it out when you have the chance?

I can't run the full pre-commit on my system (nodejs error), so if you could pull down my branch and run the pre-commit that would be great.

Also the tests are failing rn. Not entirely sure why going to work on that of course before I switch it from a Draft to Full PR.

abkfenris commented 1 year ago

I just started taking a look. At a quick glance at the failures, a bunch are from using | for unions which is syntax that got added in 3.10, and we are still supporting 3.9.

For the pre-commit, you can make a comment with "pre-commit.ci autofix" on the PR, and pre-commit.ci will push the changes from Ruff back onto your branch and you can pull those back down. https://results.pre-commit.ci/run/github/235846200/1689788417.B7uBcS6vRRy61WJYv_tI8Q

xaviernogueira commented 1 year ago

Should we issue a bug fix release so it can auto build PyPI? Or do you want to wait for the backwards compatibility tests to be added?

abkfenris commented 1 year ago

Yup, I'll try to clean up some of the other relevant PRs and make a release.

Closed by #215

abkfenris commented 1 year ago

I tried cutting a release, but PyPI has made some changes that has caused it to fail. I'm working with Joe on getting them fixed.

xaviernogueira commented 1 year ago

Sounds good. In case it's useful have a release -> PyPI workflow that worked very recently here.

We used different build commands:

    - name: Build package
      run: python -m build --sdist --wheel . --outdir dist

    - name: Publish package
      uses: pypa/gh-action-pypi-publish@27b31702a0e7fc50959f5ad993c78deac1bdfc29
      with:
        user: ${{ secrets.PYPI_USERNAME }}
        password: ${{ secrets.PYPI_PASSWORD }}
abkfenris commented 1 year ago

There were some pretty similar steps previously, but it looks like PyPI just made more folks need to have 2 factor auth enabled. Instead I've moved things to use the trusted publisher system, and ended up moving from setup.py to pyproject.toml at the same time (can't remember how to fix things in setup.py, but can remember how to migrate them).

Mind taking a look? #218

xaviernogueira commented 1 year ago

Just seeing this now. Seems like you made the merge? Let me know if you still want me to look at it.

abkfenris commented 1 year ago

Thanks, hopefully this time will work, otherwise I may ping you to get more eyeballs on why the release isn't working.

abkfenris commented 1 year ago

No need for the bat signal, 0.3.2 has hit PyPI.

On the conda side of things, that will hopefully happen within the next 24 hours, and @ocepaf has created a repo patch so that it won't try to install earlier versions of Xpublish with Pydantic v2.