Some ideas for improvements (performance + maintainability)

xaviernogueira commented 1 year ago

Hi all. First off great work on this repo @falkephi et al.

I am in the process of deploying a large-scale, real-world server using opendap-protocol (via xpublish-opendap) and am looking to squeeze some extra performance + maintainability out of this repo via a PR.

Before I write/edit code and submit a PR I wanted to gage the community's interest in having me do so. Are we happy with the project as is? Or would you be willing to review a PR with substantial amounts of restructuring?

I am not planning to change functionality, here are my potential avenues of attack:

Encapsulate the data schemas with pydantic or dataclasses (feel free to add input on a preference, 3rd party vs native python).
The level of inheritance is a bit confusing and repetitive -> I will be looking for opportunities to simplify it.
Since most of the functions are generators (via yield style returns), we can refactor to leverage Python Iterator classes that are more explicit about the generated returns.
Split out things into multiple .py files (i.e., Atom types should be in their own namespace).

I am not sold on these ideas but will play around with them and see how things look. I just wanted to check that this work won't be in vain and that there is someone on the other end willing and able to review such a PR. Thanks!

jhamman commented 1 month ago

@falkephi, @gabrielaznar, @matthieu-bernard - curious if any of you can comment on the status of this project. Should we consider this project abandoned?

jhamman commented 1 month ago

@dnerini and @fpavogt -- do either of you have any knowledge as to the status of this project?

xaviernogueira commented 1 month ago

@jhamman idk if you saw my other comment, I thought I sent it but I guess it didn't go thru.

But I wanted to mention I feel that whether or not you fork this, I think the protocol should be implemented in it's own repo (not pulled into xpublish-opendap). This is because it's an array generalized approach (dask/numpy), and it is the best example of a Python implementation, which has relevance to the scientific community broadly.

Also there is value in having the versions decoupled.

dnerini commented 1 month ago

@dnerini and @fpavogt -- do either of you have any knowledge as to the status of this project?

Hi @jhamman and @xaviernogueira, (apologies for having missed on this issue when you submitted it).

The project isn't actively maintained, but it's still being used internally, although I believe the current plan is to deprecate it. I'll try to get a more accurate answer internally and will get back to you.

jhamman commented 1 month ago

Thanks @dnerini -- good to know! The Xpublish developers would be happy to take over maintaining this project as it is used in xpublish-opendap.

One particular challenge I see is that the PyPI distribution seems to have only one owner/manager (@falkephi) who seems to have moved on. If there isn't a way to get passed that, we will need to create a new package on PyPI.

petermkr commented 1 month ago

Hi @xaviernogueira and @jhamman,

thanks a lot for your interest in this repository and the kind offers. Sorry that the issue initially slipped through.

@xaviernogueira If you want to contribute a MR, I'd be happy to review and merge it. If your plans have changed and you want to take over this project, I can definitely look into ways of doing so.

Regarding PyPI, I'll see what I can do.

falkephi commented 1 month ago

Hi everyone.

I have left MeteoSwiss a while ago. I'm willing to hand over all PyPi related stuff. Let me know what I can do to help you.

fpavogt commented 1 month ago

Re. PyPI, one option could be to:

handover the ownership of the package (on PyPi) to the user "MeteoSwiss" (https://pypi.org/user/MeteoSwiss/)
(if needed/applicable) use existing PyPI tokens stored as secrets of the MeteoSwiss Github organization to automate future releases of the repo.

There may be other better options for this specific repo - just thought I'd mention this one.

xaviernogueira commented 1 month ago

@petermkr I am probably not the guy to take it over to my personal account, however I would be happy to help maintain it.

I feel like xpublish-community could maybe take it @jhamman ? Or another org especially with contributors from different employers. Another option could be the USGS, which funded my work on this project, and currently uses this project on production servers.

Happy to help make connections if you want to pursue the USGS ownership route, but (selfishly) I lean to xpublish-community as that would make it easier for me to contribute without jumping thru any hoops.

abkfenris commented 4 weeks ago

We could take it within https://github.com/xpublish-community/ We also have a PyPI organization to manage releasing packages https://pypi.org/org/xpublish/

falkephi commented 4 weeks ago

I am happy with the project being taken over by the xpublish community. MeteoSwiss should have the last word on this, depending on the package use in production code.

For creating a package a new solution needs to be found, as my PyPi account seems to be blocked because of an outdated primary email address.

petermkr commented 3 weeks ago

I'm currently looking into the modalities of transferring the repo. I'll keep you posted.

abkfenris commented 3 weeks ago

If Joe or I can get added as admins to this repo, we can transfer into xpublish-community (or transfer to us and we can then move to xpublish-community). https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository#repository-transfers-and-organizations

On the PyPI side, there is a PEP defining transferring projects. https://peps.python.org/pep-0541/#how-to-request-a-name-transfer While this doesn't fall squarely into one of the categories the PEP defines, I think this issue probably would be enough to get them to transfer without needing to recover the account.

jhamman commented 1 week ago

Bumping this issue.

If transferring management of the repo/pypi is not feasible or a major hassle, we can move forward with vendoring this project in xpublish-opendap. It seems like we're the only user of this project anyway so it really may be simplest for us to just take the code over there.

@petermkr, @fpavogt - what do you think?

petermkr commented 1 week ago

@jhamman Give me two more weeks, I just want to ensure that we have waited long enough so that everyone had the chance to notice that we're planning to transfer the repo (it's vacation time for some). So far it's looking like we can transfer the repo to you.

Cheers, Peter

falkephi commented 1 week ago

@petermkr : When I left MeteoSwiss this library was used in production code (data4web). You should make sure a transfer does not affect production systems. You should get in touch with the relevant teams.

xaviernogueira commented 1 week ago

@aufdenkampe @DOI-USGS @sjordan29 @ptomasula Catalog-To-Xpublish should be unaffected since it uses xpublish-opendap to resolve this dependency, but just keeping you all aware about changed here.

MeteoSwiss / opendap-protocol

Some ideas for improvements (performance + maintainability) #9