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

phase 3 / as-needed: update SmartAPI yamls for BTE + Service Provider #594

Closed
colleenXu opened this issue Mar 28, 2023 · 13 comments
Closed
Assignees

Comments

@colleenXu
Copy link
Collaborator

colleenXu commented Mar 28, 2023

The SmartAPI yamls for BTE + Service Provider are based on the TRAPI spec, so they'll need updating to TRAPI 1.4. The info.x-trapi.version value will change to 1.4.0.

  • timing: later, during the deployment process
  • priority: lower
  • responsible dev: me

Some things are a bit unclear to me, but they're not big deals...

  • when to do these updates, given the gradual updating of instances/servers.
  • how to report what TRAPI version the different instances/servers are on (related to the discussion here)

one solution is to do what we did in the TRAPI 1.3 rollout: separate yamls for TRAPI 1.3 and 1.4 and we put some server/instance urls in one yaml and some in the other.

Will cover these changes

From the the changelog

@colleenXu colleenXu self-assigned this Mar 28, 2023
@colleenXu colleenXu changed the title trapi 1.4: update SmartAPI yamls for BTE + Service Provider update SmartAPI yamls for BTE + Service Provider Mar 29, 2023
@colleenXu
Copy link
Collaborator Author

Updated links + details due to updates to TRAPI 1.4 during this "beta phase".

When changes to TRAPI 1.4 happen, they should be on this branch and documented in this branch's changelog

@colleenXu colleenXu changed the title update SmartAPI yamls for BTE + Service Provider phase 3 / as-needed: update SmartAPI yamls for BTE + Service Provider Apr 4, 2023
@colleenXu
Copy link
Collaborator Author

Note: TRAPI is currently on v1.4.0-beta4 -> would update to this or whatever is the latest version when it's time to

@colleenXu
Copy link
Collaborator Author

SmartAPI yaml bits that I need to ask others about...

for @tokebe:

  • BTE's version is "2.8.1", which I think comes from the npm versioning which is defunct...Do you suggest using some kind of new versioning with BTE / putting something else there?
  • info.x-trapi section: could you review this vs the TRAPI spec, and suggest edits if needed? Some edits I could think of are...
    • adding a batch_size_limit
    • adjusting the rate_limit
    • editing the comments
  • servers section's comments (on what these instances are and how they're deployed): could you review them and suggest edits?
    • perhaps this info doesn't need to be in this yaml, and instead it's somewhere else in documentation?
  • are there BTE-specific parameters for the endpoints, that we want to advertise in this yaml? I'm thinking of optional parameters that aren't smartapi_id / team_name for the api-specific/team-specific endpoints. Right now...
  • check_query_status endpoint: does this endpoint spec need adjustments? I dunno if this endpoint is still accessible in our TRAPI 1.4 instances or if its behavior has changed...
  • asyncquery_status endpoint: does this endpoint spec need any adjustments (aka BTE-specific stuff)?

Questions for Translator:

@colleenXu
Copy link
Collaborator Author

colleenXu commented May 13, 2023

Changes made so far:

Now BTE + Service Provider show up as having TRAPI 1.3 and 1.4 instances

Screen Shot 2023-05-12 at 9 38 18 PM

Screen Shot 2023-05-12 at 9 38 02 PM

@tokebe
Copy link
Member

tokebe commented May 15, 2023

Note that in the new yaml, check_query_status should be removed as it is no longer supported. We may wish to detail the operation of asyncquery_response as well, but that's entirely optional as it's technically only TRAPI-compliant when accessed via a response from asyncquery_status.

@colleenXu
Copy link
Collaborator Author

@tokebe

I've removed the /check_query_status endpoint from all the TRAPI 1.4 yamls: NCATS-Tangerine/translator-api-registry@744c942 and fd9bf29

I know that your other point on asyncquery_response and my questions above haven't been addressed yet. I think they're lower-priority but may be good to discuss sometime...

@tokebe
Copy link
Member

tokebe commented May 17, 2023

I'll try to respond to those in order:

  • Since we haven't dealt with versioning for a while, there isn't a very straightforward answer here, especially because I haven't been able to find anything that directly relates to that version number. I think we should go of our repository, with the last tag being v1.3.0 -- based on the rules of semantic versioning, we'd probably want to bump up to v2.0.0 for TRAPI 1.4. That said, we don't really have a release system in current use, so there's room for discussion about this (and how to manage versioning moving forward). Tagging @andrewsu for opinions.
  • Rate limit (and associated comments) is already correct, I'd recommend adding a batch size limit of 200.
  • Comments on servers already look fine. Maybe add something about ITRB requiring a Change Request to deploy to Test/Prod from Staging?
  • asyncquery_response accepts a urlquery log_level which filters the logs to the given level or higher, other than that, I don't think so...
  • asyncquery_response is otherwise the same as asyncquery_status, but: if the job is done, it instead responds with literally just what a synchronous TRAPI response would look like.

@colleenXu
Copy link
Collaborator Author

colleenXu commented May 22, 2023

Update:

  • registered yamls have been updated to advertise that CI instances have been updated to TRAPI 1.4 (and registrations have been refreshed) NCATS-Tangerine/translator-api-registry@6e78adf
  • also updated the tool versions NCATS-Tangerine/translator-api-registry@45693df and internal lab slack thread
  • I didn't update the local copy of the SmartAPI yaml in biothings_explorer. I don't think it will cause issues to leave this not-updated for now (wait until all instances are on TRAPI 1.4? or some other update is needed to this spec?)

@colleenXu
Copy link
Collaborator Author

colleenXu commented May 23, 2023

The TRAPI 1.4 instances will be released as another tool version (4.0.0)

Copied from internal lab slack thread

Jackson Callaghan
Regarding deployment of TRAPI 1.4 to CI: We obviously are going to want a 1.3 branch that holds the current main prior to the 1.4 merge. I was wondering if we wanted a tag? I checked and it looks like we haven't used tags since 2021, so I just wanted to check if we cared about version tagging at all?

andrew
in principle that sounds like a good idea to me. But I defer to you and Chunlei on if there is a better way to accomplish that intent...

Jackson Callaghan
For now I'll tag it based on semantic versioning against the previous tag...we can decide if we want to do anything different later

chunleiwu
👍 good with me on this plan

chunleiwu
previously, Kevin makes official release to docker hub, that tag should match each release, but we did not continue doing that (after workspace conversion)

Jackson Callaghan
Note that I'm rolling a lot of tags up a major version because things have had breaking changes either in trapi versioning or in internal api (moving to records class, etc)

colleenxu
hmmm note that I updated the "version" for BTE to 3.0.0 in the SmartAPI yamls since that's the latest tag for the biothings_explorer repo.
However, I see that 3.0.0 = 1.3 branch. So once we get all instances to trapi 1.4 (and that code is kinda stable?), maybe we'll want another tag?

Jackson Callaghan
Yeah, TRAPI 1.4 will bring us to BTE 4.0.0 probably

@colleenXu
Copy link
Collaborator Author

colleenXu commented Jul 10, 2023

Updated TRAPI specs to 1.4.2 using NCATSTranslator/ReasonerAPI@v1.4.0-beta...v1.4.2

EDIT with note: TRAPI version should still be reported as 1.4.0 (Translator Slack link) even though we actually updated our specs to 1.4.2. Update 2023/07/25: confirmed to keep doing this (Translator Slack)

@colleenXu
Copy link
Collaborator Author

This issue is basically complete:

What is still hanging:

Going to leave this issue open under my to-dos....until this last task is resolved...

@tokebe
Copy link
Member

tokebe commented Aug 3, 2023

@colleenXu I leave it to your judgement whether we should close this and track the few remaining stray parts in a new issue, or just continue using this issue to track it (or if they're low-enough priority that they don't really need tracking...).

@colleenXu
Copy link
Collaborator Author

colleenXu commented Mar 26, 2024

Closing. Linked the old questions/comments in the new issue, for potentially reviewing as part of that effort.

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

No branches or pull requests

2 participants