Skip to content

Conversation

@GBirkel
Copy link
Collaborator

@GBirkel GBirkel commented Dec 12, 2025

Objects of the appropriate types are required and returned for dataset create, dataset update, sample create, sample update, instrument create, instrument update, proposal create, proposal update, and datablock create. There are now endpoints for deleting samples, instruments, and proposals. More accurate models as well.

This PR should represent a version # change for Pyscicat, becayse it will probably break existing code.

For example, instead of this:

dataset = client.datasets_get_one(pid=dataset_id)
assert dataset is not None
logger.info("Get response: %s" % json.dumps(dataset, indent=2))
print(dataset["pid"])

We now do this:

dataset = client.datasets_get_one(pid=dataset_id)
assert dataset is not None
logger.info("Get response: %s" % dataset.model_dump())
print(dataset.pid)

Because we're getting back instantiated class objects with real attributes, not just generic objects.

…d returned for dataset create, dataset update, sample create, sample update, instrument create, instrument update, proposal create, proposal update, and datablock create. There are now endpoints for deleting samples, instruments, and proposals.
…ned on a call to an API endpoint (.../v100/...) that doesn't exist, but what it's actually testing is the "datasets_get_one" call, which has traditionally returned a 404 when the criteria given to the endpoint do not match any records. That is not a 404. That is a successful HTTP call to a valid endpoint, which does not garner a 404 status code. Instead the endpoint should return a response indicating there was no matching record. Pyscicat now returns 'None' for that case, which is much more appropriate than a 404 exception. That change now makes this test fail, because this test expects a 404. This test is also bogus because the client would not even be able to authenticate, let alone make a request for a dataset, if the base URL provided was invalid, so a user would never reach this state.
@GBirkel
Copy link
Collaborator Author

GBirkel commented Dec 12, 2025

about test_get_dataset_bad_url:

This test no longer applies. It's meant to ensure that a 404 is returned on a call to an API endpoint (.../v100/...) that doesn't exist, but what it's actually testing is the "datasets_get_one" call, which has traditionally returned a 404 when the criteria given to the endpoint do not match any records. That is not a 404. That is a successful HTTP call to a valid endpoint, which does not garner a 404 status code. Instead the endpoint should return a response indicating there was no matching record. Pyscicat now returns 'None' for that case, which is much more appropriate than a 404 exception.

That change now makes this test fail, because this test is expecting a 404 on an attempt to call an invalid endpoint (which it is not doing). This test is also bogus because the client would not even be able to authenticate, let alone make a request for a dataset, if the base URL provided was invalid, so a user would never reach this state.

@dylanmcreynolds
Copy link
Member

Is this something you need for a project? As we discussed I think pyscicat should move towards being an implementation of the auto-generated SDK with extra bells and whistles, and not spend a lot of time improving the existing API...especially for breaking changes like this.

@GBirkel
Copy link
Collaborator Author

GBirkel commented Dec 16, 2025

This is code I put together all at once a few weeks ago at the tail end of the last Pyscicat PR, but deliberately set aside. It's not required for any projects, current or upcoming.

It uses pydantic to strictly verify the type of what's coming back from the server and return actual objects (including lists of objects). It's arguably the way the SDK should have worked from the start, but now we've got a userbase to think of, so it may never see the light of day. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants