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

feat: draco server (#394) #403

Merged
merged 35 commits into from Oct 20, 2022
Merged

feat: draco server (#394) #403

merged 35 commits into from Oct 20, 2022

Conversation

peter-gy
Copy link
Collaborator

@peter-gy peter-gy commented Sep 20, 2022

Goals

Adds a FastAPI submodule inside the draco module to expose the main features of Draco through REST endpoints. I implemented a basic layered architecture (model - service - controller) to provide reasonable testability and extensibility.

Notes

  • The server can be started by executing make serve. OpenAPI docs are accessible through http://localhost:8000/docs.
  • This PR still misses some key features, I would implement them after a brief discussion of the progress so far, to make sure that I am heading in the right direction

closes #394

@peter-gy
Copy link
Collaborator Author

peter-gy commented Sep 20, 2022

Points to discuss

Endpoints

Currently, all public methods of draco.Draco are exposed through endpoints as well as draco.run.run_clingo is supported to allow solving arbitrary ASPs. Should we expose utility methods too from draco.fact_utils? Can you think of further endpoints to be implemented?

Deployment

I was thinking about creating a simple Dockerfile to facilitate the deployment. Should we push the created image to an image registry, or do we just focus on having a server instance up and running on some infrastructure and we can skip DevOps overengineering?

API Docs and Custom Errors

As soon as we finalize which endpoints we support for now, I would invest some more time into extending the OpenAPI docs as well as creating some more custom errors to indicate common

API Testing

For now, I just used fastapi.testclient to test the actual endpoints of draco.server.controller. Should I create a Postman / Insomnia / collection of example requests in addition to it? Such a collection might be also useful as a kind of documentation of the server for devs exploring the API. (OpenAPI also makes it possible to send example requests, but the payload is always statically set to the wrapped python methods' default values, making it inflexible for exploration)

Serializing Clingo Symbols

For now, I added a very primitive serialization logic for clingo.Symbol just to have a default JSON representation to it that we can return to the client when they POST to /run-clingo or /complete-spec. All I did in draco.server.utils for now is:

def clingo_symbol_to_jsonable_symbol(symbol: clingo.Symbol) -> ClingoSymbol:
    return {"type": symbol.type.name, "value": str(symbol)}

What do you think, what other attributes of clingo.Symbol should be added here? In general, how detailed should a clingo result be described? Maybe a structure similar to that discussed in domoritz/clingo-wasm#229?

@peter-gy peter-gy added the enhancement New feature or request label Sep 20, 2022
@@ -79,3 +79,8 @@ clean:
@find . -type d -name '*pytest_cache*' -exec rm -rf {} +
@find . -type f -name "*.py[co]" -exec rm -rf {} +
@find . -type d -name '*.ipynb_checkpoints' -exec rm -r {} +

.PHONY: serve
serve:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can someone run the server if they install Draco via pip? We should document that.

@domoritz domoritz marked this pull request as draft September 27, 2022 21:06
@domoritz domoritz removed the draft label Sep 27, 2022
It was not possible to build `jupyter-book` otherwise.
Kept getting the error `sphinx-togglebutton 0.3.2 requires wheel, which is not installed`
Excluded unreachable lines from coverage.
Endpoints defined in `_register` are tested, however
they are not included in the coverage report, probably
due to the dynamic way how they are declared.
@peter-gy peter-gy marked this pull request as ready for review October 8, 2022 17:13
@peter-gy
Copy link
Collaborator Author

peter-gy commented Oct 8, 2022

Hey @domoritz!

As soon as we finalize the endpoints and their signatures (that is, what is the JSON input we expect from users) I'll create detailed docs about the server in jupyter-book.

You can explore the current state of the server through the OpenAPI UI by executing python -m draco.server then opening http://localhost:8000/docs.

Feel free to suggest changes to the endpoints if you think that different input-output behaviour would lead to a better UX/DX. The server is highly extensible and pretty easy to modify. We (and users installing draco via pip) can always add or remove endpoints by adjusting the base_routers param of the DracoAPI instance.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I love how I can try this online at http://127.0.0.1:8000/docs/. I will merge this but it would be good to add some docs to the website as well to tell people how to use the server.

@domoritz domoritz merged commit 331157b into main Oct 20, 2022
@domoritz domoritz deleted the feat/draco-server branch October 20, 2022 16:50
@peter-gy
Copy link
Collaborator Author

This is great. I love how I can try this online at http://127.0.0.1:8000/docs/. I will merge this but it would be good to add some docs to the website as well to tell people how to use the server.

Thanks for the review, I opened #422 for docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a web server that can run Draco
2 participants