lsst-uk / lasair-lsst

Apache License 2.0
0 stars 0 forks source link

sherlock api with multiple positions #133

Open gpfrancis opened 4 months ago

gpfrancis commented 4 months ago

The sherlock object api handles lists of objects whereas the sherlock position api only handles a single position (despite the underlying sherlock service supporting lists for both). This is inconsistent and non-intuitive.

gpfrancis commented 1 week ago

From Slack:

Gareth: I don't see a good use case for doing multiple queries this way. If someone wants to look up the Sherlock classificaation for a list of objects that are already in the database then query is much quicker than Sherlock. But if there is a case for doing many queries then it's much more efficient to submit them in batches. Either way, I think the two sherlock methods should be consistent with each other.

Roy: The only person I know who has done multiple sherlocks is Lydia, who was just pushing a big catalogue of her own though sherlock ....

Gareth: I think Lydia was only after the top classification for each object though, which is solved more efficiently by a database query without needing to re-run sherlock at all.

RoyWilliams commented 5 days ago

It would be nice to have a single Lasair client to access both ZTF and LSST. But this issue breaks that model. Looking to the future, we can change sherlock_object from multiple to single for LSST. But now that we have agreed to keep ZTF into the indefinite future, then surely the same should be for ZTF. Are we (a) Ready to make a significant change to the ZTF REST and API at this time? Or (b) Maintain different REST/client for ZTF and LSST?

RoyWilliams commented 5 days ago

Or of course (c) just leave it with multiple object / single position

gpfrancis commented 5 days ago

My suggestion:

/sherlock/position/ is fine since it already has the desired behaviour. /sherlock/objects/ should keep the same behaviour, but be deprecated in Lasair-LSST (and ultimately removed) /sherlock/object/<objectId> new method in Lasair-LSST that gets a single object

The proper way to handle this of course is with API versioning, but we could probably just fudge it in this case. Perhaps by making the object method in the python client have different implementations depending on the endpoint it is using.

RoyWilliams commented 5 days ago

I am trying to keep the same API and client for both ZTF and LSST. If we add a new method to one, we need to add it to both. Thus we have a branch/testing/pullrequest on both ZTF and LSST codebases. Is this what you are suggesting?

gpfrancis commented 5 days ago

I'm saying that I think the correct answer is not to try to keep the API and client exactly the same forever, but to have versions. However, I also agree that that is maybe a larger step than is justified by this alone when we can probably fudge the issue in an acceptable way.

Adding the new method to both old and new APIs and clients is probably the cleanest such fudge, but yes that would require work on both codebases.

A less clean, but also lower effort option would be to add the new API method to Lasair-LSST only and add an object method to the client code which uses the new API method by default, but detects if it's talking to the old API (either by looking at the URL or by catching the error when it tries the new method and retrying with the old one). Thus we would have two APIs, but a common Python interface for clients.

RoyWilliams commented 5 days ago

Like this you mean?

    def sherlock_object(self, objectId, lite=True):
        try:
            input = {'objectId':objectId, 'lite':lite}
            result = self.fetch('sherlock/object', input)
        except:
            input = {'objectIds':[objectId], 'lite':lite}
            result = self.fetch('sherlock/objects', input)
        return result

    def sherlock_objects(self, objectIds, lite=True):
        """ DEPRECATED """
            input = {'objectIds':objectIds, 'lite':lite}
            result = self.fetch('sherlock/objects', input)
        return result
gpfrancis commented 5 days ago

Something like that, yes. You probably want to specify the exception you expect to get.

I still think that to be a REST api rather than just an HTTP api it should be /object/12345 rather than /object/?objectId=12345.