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

Adds draft backend API, db integration, db schema migration #5

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

falquaddoomi
Copy link
Contributor

This PR replaces the "dummy" API from the previous PR with one that does the following:

  • interacts with the postgres server to store and retrieve analysis objects, served from the analysis endpoint
  • implements other very WIP endpoints, e.g. stats, just for testing
  • adds database schema migrations via Atlas
  • adds more verbose JSON serialization (to get around Plumber just crashing when it encounters unserializable values)

Other non-API-related changes:

  • adds WIP test harness that allows tests to be located in subdirectories
  • adds shell control command to run_stack.sh that starts a shell in the backend container

Things you might want to test:

  • ensure that everything you need to create your .env file is in .env.TEMPLATE
  • ensure that you can start the stack by running ./run_stack.sh
  • ensure you can see the API at http://localhost:9050
  • try making a POST request to analyses, possibly via the Swagger UI at http://localhost:9050/__docs__/
  • after the stack is up, try running ./run_stack.sh shell in another window to get a shell into the backend
    • try running the test harness script from there, e.g. ./run_tests.sh

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for molevolvr failed.

Name Link
🔨 Latest commit b2c839d
🔍 Latest deploy log https://app.netlify.com/sites/molevolvr/deploys/66eb4ba40cb8ac0008cb085a

@vincerubinetti
Copy link
Collaborator

vincerubinetti commented Sep 18, 2024

Looking at this now.

Do you know if Plumber can export OpenAPI types? If so, I could automatically generate typescript types for everything. I found this page but I can't tell if it's Plumber source code, a tutorial/example, a Plumblr plugin, or something else completely.

Edit: Nevermind, I forgot that Swagger needs (I think) an OpenAPI schema to work, and you already have that set up. I see the schema at http://localhost:9050/openapi.json I'll change my question to, can we export that to a folder in the repo so I can read it statically from the frontend build tools without having to run the backend?

Also just curious about the docs endpoint being /__docs__. Is that a Plumber requirement or something? Is there a naming conflict we're worried about here?

Another edit: I see now that's just a default by Plumber I guess. Fine to leave that as is.

@vincerubinetti
Copy link
Collaborator

vincerubinetti commented Sep 18, 2024

Okay, looks like you could call getApiSpec. Not sure if this returns a path or the actual json of the spec (docs aren't clear about that...), but you could write that to e.g. /backend/api/spec.json (my recommendation). The backend would still need to run to generate this, but now that I think about it there's probably no way around running it.

If you want to add that into this PR, I could also setup the typescript generation here too. Hopefully shouldn't be too much effort. Unlike in past PRs, I think this would be actually relevant to the subject of the PR to tack on.

@vincerubinetti
Copy link
Collaborator

Other than my above comments, this seems to work as expected, though I have no experience with Plumber. Maybe Dave or David could look over this?

@vincerubinetti
Copy link
Collaborator

vincerubinetti commented Sep 18, 2024

after the stack is up, try running ./run_stack.sh shell in another window to get a shell into the backend

It works for me. Very cool. One last thought, maybe we rename /app to /frontend, and then use "app" to refer to the whole stack. I saw /app in the Plumber source code, and I was a little confused why it was referring to the frontend, but now I figure that has something to do with it all being mounted in /app when you shell into the container?

We're referring to the backend as "backend" so "frontend" would be consistent with that too.

@falquaddoomi
Copy link
Contributor Author

It works for me. Very cool. One last thought, maybe we rename /app to /frontend, and then use "app" to refer to the whole stack. I saw /app in the Plumber source code, and I was a little confused why it was referring to the frontend, but now I figure that has something to do with it all being mounted in /app when you shell into the container?

Right, I can see how that's confusing...kind of out of habit, I put application-specific code in a container in a folder in the container's filesystem called /app; it's not referencing the frontend.

I do like the idea of changing the app folder in the repo to frontend, to clarify that it's the frontend and not the whole stack.

@falquaddoomi
Copy link
Contributor Author

falquaddoomi commented Sep 18, 2024

Okay, looks like you could call getApiSpec. Not sure if this returns a path or the actual json of the spec (docs aren't clear about that...), but you could write that to e.g. /backend/api/spec.json (my recommendation). The backend would still need to run to generate this, but now that I think about it there's probably no way around running it.

Right, the OpenAPI spec is generated via reflection at runtime, which means the backend has to be running. I'll pull the OpenAPI spec as a file and add it to this PR. About the name of the spec file, I'd personally prefer openapi.json so it's clear where it came from, but I don't know if Plumber will automagically read it or not. I'll leave it as /backend/api/spec.json as you've requested for now and perhaps change it in the future.

I will say that the OpenAPI spec Plumber generates by default has limited usefulness; R is famously loose with its typing, so Plumber can't extract much of a schema for the return value from the API, and it can barely do it for the parameters, too, since base R doesn't support type hinting. Plumber's alternative is for you to add an annotation to each endpoint, #* @response <name> <openapi-formatted response> that it then basically dumps back into the OpenAPI spec.

Here are two possible options:

  1. We manually create the OpenAPI spec as JSON/YAML, instruct Plumber to use it via set_api_spec(), and ensure the actual API conforms to it by calling validate_api_spec().
  2. I add the #* @response annotations to each endpoint as Plumber describes in its block annotations docs, we pull the resulting OpenAPI spec file, and put it somewhere in the filesystem for the frontend build tools to find.

Option 1 is more straightforward IMO and makes the OpenAPI spec the source of truth rather than an artifact. While option 2 makes the code more verbose and requires a step where the spec is extracted into a file, there's something to be said for the response documentation being near the code that generates it.

@vincerubinetti
Copy link
Collaborator

A third option is just to not deal with a spec at all, and I'll just manually make my typescript as I was planning to do anyway.

Do you think it would
help you at all on the backend to have a spec? If you get little or no value from that, I'd vote for option 3.

@vincerubinetti
Copy link
Collaborator

Let me try converting the dump you just pushed to typescript and seeing how useful it is as well.

@vincerubinetti
Copy link
Collaborator

Let me try converting the dump you just pushed to typescript and seeing how useful it is as well.

Okay I think it may be of some help to me, even if it's just for getting the names of the endpoints and being able to get a TS API client out of it. I'm thinking I'll probably use it, even if I have to supplement it with hand-written types.

I get the benefits of option 1, but I think writing an OpenAPI spec by hand for all of the endpoints and complex data structure's we're going to have is going to become a burden. It's fairly verbose and repetitive. Option 2 seems like it's a Plumber special syntax that's a little more terse? And I'm assuming since it gets converted to a spec anyway, it has the same level of checking (R type hinting? compile warnings?) as option 1, if any at all.

I suppose ultimately, someone is going to have to do the work of defining the structure of this data, it's just a question of who. I'm happy to take on that task (especially since the frontend is enforcing this requirement by using TS), especially if doing the type annotations at the backend level doesn't really benefit you in any way. Would it help to have those annotations collocated with those endpoint functions? I feel like most of the structure info is really going to be coming from the package level anyway. Having the Swagger UI be more descriptive and type-safe would be a nice benefit though

Maybe I just try out manually converting the spec to TS for a while -- seeing how useful and accurate it is -- before we make the effort to do one of the options and/or automate the TS conversion.

In any case, let's hold off for this PR at least, this is getting too in the weeds. I might copy this discussion to an issue.


Regarding "app" -> "frontend", since you said you have some upcoming changes relevant to this stuff, maybe we can wait until that's done before renaming the folder and associated docker commands. Possibly in a new PR.

@falquaddoomi
Copy link
Contributor Author

A third option is just to not deal with a spec at all, and I'll just manually make my typescript as I was planning to do anyway.

The third option sounds good, at least for now. Maybe once things have crystallized a bit more we can go back to looking at a server-side spec, if only for documenting the API and for validation, not necessarily for your frontend.

@falquaddoomi falquaddoomi merged commit f669015 into main Sep 24, 2024
@falquaddoomi falquaddoomi deleted the backend-draft-api branch September 24, 2024 17:07
@falquaddoomi falquaddoomi mentioned this pull request Sep 25, 2024
falquaddoomi added a commit that referenced this pull request Oct 3, 2024
This PR builds on #5, with two main objectives:
- add SLURM, config, and supporting services to the stack
- integrate async job processing into the backend via SLURM

In addition to the main objectives, this PR includes a few tweaks to API
response handling and the testing framework. A new field, `reason`, has
been added to the `analyses` table to capture why a status was set.
Currently it gets set to an exception traceback if the analysis throws
an error.

The PR includes a skeleton of how analyses would be processed in
`backend/api/dispatch/submit.R` (called via `dispatchAnalysis()` in
`backend/api/endpoints/analyses.R`), but it doesn't actually do any real
work yet.

Things to try:
- bringing up the full stack and seeing if it builds and runs without
issue
- submitting analyses and seeing if the status eventually gets updated
- trying the tests: after launching the stack, execute `./run_stack.sh
shell` in a separate window, then run `./run_tests.sh`, which will
perform a few integration tests
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.

2 participants