search

eth-cscs / pyfirecrest

Python wrappers for the FirecREST API
https://pyfirecrest.readthedocs.io
BSD 3-Clause "New" or "Revised" License
10 stars 7 forks source link

A version control could be used across the interface #116

Closed khsrali closed 3 weeks ago

khsrali commented 3 months ago

As far as I understand self._api_version is defined but not used.

It would nice, if there was a check on functionalities that are not available in all version. If the api version is outdated, return NotImplementedError or even better a custom "NotImplementedOnAPI" for those methods.

Example: FirecREST v1.15.0 for example, doesn't support recursive ls. in this case, if pyfirecrest is instantiated, with self._api_version=1.15.0 it should return NotImplementedOnAPI when list_files(recursive =True)

(this issue is indirectly related to https://github.com/eth-cscs/firecrest/issues/204)

ekouts commented 3 months ago

Hm, indeed right now pyfirecrest doesn't really use this much. The only case where it is used, is for some incompatibility issue with the object storage link in external transfers. When an endpoint is new then in theory the client can easily catch the 404 result from the server, but when new arguments are added then they are silently ignored (as you mentioned in the recursive ls). I could go through the different versions' release notes and implement this, but to be honest it's a bit low in my current priorities and with the holiday season I would say that this may be ready by the end of August. I hope this is okay 😅

khsrali commented 3 months ago

I opened a PR on this, only because already had a look, and was half the way through... If you don't like the approach, feel free to solve it in a different way :) But I'm not in a hurry no worries :)