Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving NodeNorm TRAPI prefixes from the ingress rules into the application itself #266

Open
gaurav opened this issue Jun 17, 2024 · 4 comments

Comments

@gaurav
Copy link
Contributor

gaurav commented Jun 17, 2024

NodeNorm currently acts like a TRAPI application: the Smart API documentation includes the standard TRAPI information, it is registered in SmartAPI as a TRAPI application, and so on.

However, only two out of our six API endpoints are actually TRAPI-related:

  • /query will normalize an entire TRAPI message.
  • /asyncquery will normalize an entire TRAPI message, then send the output to a provided callback.

All the other API endpoints are NodeNorm-specific and not related to TRAPI at all. We currently register [NodeNorm URL]/1.4/ with SmartAPI, but there's nothing stopping us from registering [NodeNorm URL], in which case we will update automatically when we upgrade to a new TRAPI version.

Our SmartAPI is currently configured such that the entire NodeNorm app is hosted at both root (1) as well as a TRAPI prefix (e.g. for TRAPI 1.4, we provide (2)). We currently provide /1.3/ and /1.4/ prefixes (#238), but do not yet provide /1.5/ prefixes (#254).

Going forward, we can do one of four things:

  1. Continue adding new TRAPI version prefix, with an explicit deprecation policy that says that we will either support two (i.e. /1.5/ and /1.4/) or three (i.e. /1.5/, /1.4/, /1.3/) prefix, and that the oldest prefix will be retired when a new one is added (i.e. adding TRAPI 1.6 will cause us to retire the TRAPI 1.3 prefix). We currently implement this not in the application but in the Kubernetes ingress: we set up ingress rules so that requests sent to /1.3/ and /1.4/ are handled by the root web workers.
    • Note that given how NodeNorm is currently designed, we can't easily support true multi-version support: if the code supports TRAPI 1.4, then we are simply providing a TRAPI 1.4 interface that is pretending to be a TRAPI 1.3 interface.
    • I don't like this because it requires careful coordination between the ingress rules in translator-devops and in the NodeNorm repo, but it's what we're already doing, so no new work will be needed.
  2. Move the TRAPI prefix into the application, but only for TRAPI-specific endpoints. This will mean that will no longer be able to access [NodeNorm URL]/query, but only [NodeNorm URL]/1.4/query for TRAPI 1.4. You will also no longer be able to access [NodeNorm URL]/1.4/get_normalized_nodes, but only [NodeNorm URL]/get_normalized_nodes. Since it is in the application, we will be able to "support" multiple TRAPI versions (i.e. if we know that the current version produces valid TRAPI 1.4 and TRAPI 1.5 but not valid TRAPI 1.3, we can turn on the /1.4/ and /1.5/ endpoints but return errors on the /1.3/ endpoints).
    • I like this as a reasonable compromise between fully supporting multiple TRAPI versions and maintaining existing URLs while using HTTP errors to point people to the right URLs. Tools using endpoints that don't need TRAPI prefixes will see errors, and will be able to update their tools to remove those. Tools that do use TRAPI endpoints can fix on a particular version that they want to support, and will get an error once that version has been deprecated.
  3. Move the TRAPI prefix into the application for all endpoints. This will mean that all of the following endpoints will work: [NodeNorm URL]/query, [NodeNorm URL]/1.4/query, [NodeNorm URL]/1.4/get_normalized_nodes and [NodeNorm URL]/get_normalized_nodes. Since it is in the application, we will be able to "support" multiple TRAPI versions (i.e. if we know that the current version produces valid TRAPI 1.4 and TRAPI 1.5 but not valid TRAPI 1.3, we can turn on the /1.4/ and /1.5/ endpoints but not the /1.5/ endpoint).
    • The biggest upside to this approach is that essentially all current NodeNorm URLs in the wild will continue to be supported, and then we can apply the deprecation policy as we increase the TRAPI version.
    • The biggest downside to this approach is that some tools will continue to incorrectly use TRAPI-prefixed URLs, but we can live with that.
  4. Fully support multiple TRAPI versions: all the TRAPI-specific code lives in normalizer.py. We should be able to split this out into TRAPI-specific modules, and then set it up so that e.g. /1.4/query handles TRAPI 1.4 only and /1.5/query handles TRAPI 1.5 only. These would largely share code between them, but with some slight TRAPI-specific changes between them. Non-TRAPI specific endpoints would remain in normalizer.py.
    • I don't think SmartAPI currently supports telling it that some APIs support TRAPI 1.5 while others support TRAPI 1.4. So we would either need to advertise ourselves as TRAPI 1.5, but support 1.4 URLs for anyone who needs them (which seems like the correct behavior anyway).
    • This will make it really easy for us to upgrade NodeNorm whenever we want: most other tools will use non-TRAPI-prefixed endpoints, and the few applications that need TRAPI-version-specific endpoints will be able to use the older or newer version as needed.
    • This would require a least a week's worth of work, so we should only pursue this if there is a significant need for multi-version TRAPI message normalization. However, once we put in the work to split this up, it'll be relatively easy to support multiple TRAPI versions going forward, so as long as expect significant TRAPI changes in the future, this will be worth pursuing.

I think we should implement "Move the TRAPI prefix into the application, but only for TRAPI-specific endpoints" for now, and then "Fully support multiple TRAPI versions" if there is either a significant need for this or the next time that TRAPI changes significantly.

@gaurav gaurav added this to the NodeNorm July 2024 milestone Jun 17, 2024
@gaurav
Copy link
Contributor Author

gaurav commented Jun 18, 2024

Note that we can implement this in FastAPI by using enumerated path components.

@gaurav
Copy link
Contributor Author

gaurav commented Jun 18, 2024

  1. Get rid of TRAPI endpoints entirely. Anybody using the /query and /asyncquery endpoints would need to move that functionality into their own tool. No part of NodeNorm-web would then need to deal with TRAPI versions.

@cbizon
Copy link
Contributor

cbizon commented Jun 18, 2024

Or 6, which is maybe less disruptive would be to keep /query and /asyncquery but drop the 1.x parts of the paths.

@gaurav
Copy link
Contributor Author

gaurav commented Jun 21, 2024

DECISION: we continue synchronizing this manually for now, but our goal is to deprecate the TRAPI-specific endpoints entirely by the Translator Fugu release (in July 2024).

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

No branches or pull requests

2 participants