PyISPyB integration: add REST implementation, corresponding to current SOAP ISPyB solution#1614
Conversation
dominikatrojanowska
force-pushed
the
maxiv-soleil-pyispyb-integration
branch
from
fe4edf4 to
e9ff884
Compare
| number: str = "" | ||
| title: str = "" | ||
| name: str = "" | ||
| state: str = "" |
There was a problem hiding this comment.
What does state mean in this context ?
There was a problem hiding this comment.
Proposal can be in closed or open state. Not sure if any other is possible here. At Max IV we are looking only for open proposals: https://github.com/mxcube/mxcubecore/pull/1614/changes#diff-6a47fcd7804afcce43cfdf9b3ce2bb8721e74ea9ede437f049b53f1f376e2d67R44
There was a problem hiding this comment.
I see, I would somehow say that get_beamline_proposals should not at all return proposals that are closed then ? Like this you would not need state
There was a problem hiding this comment.
That's another application endpoint (PyISPyB) that delivers proposals 🤔 I guess in other use cases (app's inner requests for example) it may be required to get all proposals regardless of their state. I can implement or ask about new endpoint for our purpose with the default filtering to get only those in Open state. Let me know :)
There was a problem hiding this comment.
Maybe you can just filter on retrieval so that you don't need state once everything is retrieved ? There is no need to pass it around if its not used.
There was a problem hiding this comment.
ok, removed this state field
|
Looks good overall, just a few comments to consider |
dominikatrojanowska
changed the title
dominikatrojanowska
force-pushed
the
maxiv-soleil-pyispyb-integration
branch
from
7ee0fdd to
df9b3b5
Compare
dominikatrojanowska
force-pushed
the
maxiv-soleil-pyispyb-integration
branch
2 times, most recently
from
fe9bbff to
e9a0e1e
Compare
| ) | ||
|
|
||
| @staticmethod | ||
| def __to_sample(sample_data: dict) -> dict: |
There was a problem hiding this comment.
There is nothing wrong with it - but why are you using double_underscore? As I understand it, that is specific for functions that cannot be called even from subclasses, whereas the normal syntax for private functions is a single underscore.
There was a problem hiding this comment.
That's quite right about _private and __protected members. Actually there is something called name mangling to access even the protected but generally it should be avoided.
This implementation bases on ISPyBDataAdapter and originally these helpers are protected members. I've just find no reason to change that.
There was a problem hiding this comment.
Fair enough. I have a suspicion that in many cases (possibly including this one) people are using protected members without justification, but it certainly makes more sense to leave things as they are and do the nitpicking and clean-up later (if at all).
dominikatrojanowska
force-pushed
the
maxiv-soleil-pyispyb-integration
branch
from
0b7c7b5 to
dc99421
Compare
|
Looks good as far as I can see, not sure if there are still changes requested @mockoocy. Otherwise I think we can rebase and merge |
Replace the legacy SOAP-based ISPyB client with a new PyISPyB REST implementation backed by Keycloak token authentication. Introduce ``PyISPyBDataAdapter`` and ``PyISPyBRestClient``as abstract, reusable components, removing the old ``ISPyBAdapter``. Add adjustable timeouts, improved error handling, and legacy method compatibility. Update tests to reflect the new REST implementation and handle Max IV specific ``duo`` and ``sdm`` dependencies gracefully.
Replace the legacy SOAP-based ISPyB client with a new PyISPyB REST implementation backed by Keycloak token authentication. Introduce ``PyISPyBDataAdapter`` and ``PyISPyBRestClient``as abstract, reusable components, removing the old ``ISPyBAdapter``. Add adjustable timeouts, improved error handling, and legacy method compatibility. Update tests to reflect the new REST implementation and handle Max IV specific ``duo`` and ``sdm`` dependencies gracefully.
Implement PyISPyB token refresh and implement integration tests - Add refresh token support to REST client - Add mocked API tests for PyISPyB client and adapter methods - Align adapter endpoints with PyISPyB API Co-authored-by: dominikatrojanowska <154609837+dominikatrojanowska@users.noreply.github.com> Co-authored-by: Mohsen Dahesh <mohsen.dahesh@synchrotron-soleil.fr>
…edly in _update_data_collection method
marcus-oscarsson
force-pushed
the
maxiv-soleil-pyispyb-integration
branch
from
dc99421 to
fa49625
Compare
5 participants
Replace the legacy SOAP-based ISPyB client with a new PyISPyB REST implementation backed by Keycloak token authentication. Introduce
PyISPyBDataAdapterandPyISPyBRestClientas abstract, reusable components, removing the oldISPyBAdapter. Add adjustable timeouts, improved error handling, and legacy method compatibility. Update tests to reflect the new REST implementation and handle Max IV specificduoandsdmdependencies gracefully.