FastAPI route for tags (Disabled)

[davelopez]

Ref #10889

Summary

• The legacy controller logic has been refactored into a TagManager class shared with the new FastAPI controller.

Additional comments

• I have placed the pydantic model in the manager module instead of a separate _schemas.py file because I wasn't sure where would be a better place.
• Apparently, the endpoint does not return anything. Would be better to return an empty response with a 204 status code so this is documented in the OpenAPI docs properly?
• The only test I found for this endpoint is here. It is currently failing when tested against the new FastAPI endpoint (Bad Request). I've been staring at it for a couple of hours but it doesn't fix itself neither I managed to figure out the reason, maybe another pair of trained eyes can help 😄

[jmchilton]

Would be better to return an empty response with a 204 status code so this is documented in the OpenAPI docs properly?

This sounds better than an empty 200.

[davelopez]

Update

• Return status 204 for the FastAPI endpoint (the legacy one will still return 200 with an empty JSON response). Also added an assert to the test to check the response was OK.
• To fix the Bad Request error in the new endpoint I had to change the test to send a JSON payload instead of a regular dict. Please let me know if there is a better way to deal with that issue.
• Unfortunately, the test is still failing (only for the new endpoint). This time the error is related to some kind of desynchronization in the database. Happening exactly when attaching the user to the item_tag_assoc here.

sqlalchemy.exc.InvalidRequestError: Object '<User at 0x7fb0dc422198>' is already attached to session '17' (this is '19')

I don't understand where is the difference between the two endpoint calls (legacy and FastAPI) since they both share the same class. Maybe there is some more magic happening in the legacy endpoint decorator that doesn't break the database session?

[mvdbeek]

• Unfortunately, the test is still failing (only for the new endpoint). This time the error is related to some kind of desynchronization in the database. Happening exactly when attaching the user to the item_tag_assoc here.

This is a bigger issue, we need to always work with a SessionLocal database scope and we probably should separate this from the app object. Unfortunately I don't have a more concrete suggestion here, except that you could try tracking where those 2 different sessions come from. I need to fix something else urgently and then I will look into this (if noone beats me to it).

[jmchilton]

The difference between the endpoints is probably the implementation of trans - FastAPI isn't creating a full GalaxyWebTransaction. Presumably something is happening in that constructor that results in a substantial difference. My guess would be this line:

https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/webapps/base/webapp.py#L170

I'm suspect based on Marius's comment this is not something we want to align between the endpoints though. So I'm not sure I have actionable advice without looking into it more.

[jmchilton]

Can you modify that whole tag pipeline so user can have type Union[User,str] (you might not be able to actually declare types since the tool shed might reuse this code with different user models - but the idea would be the same) and set user_id at that line instead of user if it is a string. That would be slightly more efficient and would bypass this problem. It sort of depends on us not doing something more interested with user in those functions - but we probably don't at first glace.

[davelopez]

Thank you both for the help!
I will try the Union[User,str] solution proposed and see if it works.

[jmchilton]

Actually I think I was a bit off:

def get_trans(app: StructuredApp = depends(StructuredApp), user: Optional[User] = Depends(get_user),
              galaxy_session: Optional[model.GalaxySession] = Depends(get_session),
              ) -> SessionRequestContext:
    app.model.session.expunge_all()
    return SessionRequestContext(app=app, user=user, galaxy_session=galaxy_session)

It looks like in FastAPI the session is expunged after the user is loaded instead of before. Probably just moving that expunge_all line to the top of get_session would make FastAPI behave a lot more like the traditional controllers (for better or worse, I'm unsure).

[davelopez]

It looks like in FastAPI the session is expunged after the user is loaded instead of before. Probably just moving that expunge_all line to the top of get_session would make FastAPI behave a lot more like the traditional controllers (for better or worse, I'm unsure).

I have tried to move that line, but it keeps giving the sqlalchemy.exc.InvalidRequestError: Object '<User at 0x7fb0dc422198>' is already attached to session '17' (this is '19') error. I'll try to investigate a bit further and see if I can bring some more clues.

[davelopez]

The FastAPI router has been disabled until the database session issue is fixed.

[mvdbeek]

Can we make this an enum of all taggable item classes ?

[davelopez]

Done! Great suggestion!
I've added to the enum the name of the classes that are listed in GalaxyTagHandler but I'm not sure if those are all of them or I may have missed some.

[mvdbeek]

That looks complete to me. I've been thinking about maybe using a metaclass that will register new taggable classes (or https://stackoverflow.com/a/50099920), since they should all be subclass of ItemTagAssociation https://github.com/mvdbeek/galaxy/blob/5d1d29d838983748704cc89a246799e2224b2abc/lib/galaxy/model/__init__.py#L6348

But a simple enum here is fine for now.

[davelopez]

Thanks for the hint!
I changed the Enum and now it is dynamically generated based on the names of the subclasses of ItemTagAssociation. This is a much better approach since we already found that there were some classes missing:

However, this brings some issues:

• Mypy can not correctly detect the Enum type and the only workaround to avoid the error that I found was ignoring it.
• The OpenAPI documentation does not show the enum values as it does with the regular Enums and the JSON that represents the model does not show the item_class property anymore. I'm not sure why :/

[jmchilton]

I've not encountered this pattern before, very cool.