Conversation
093a415 to
9d71874
Compare
9d71874 to
99fc1c3
Compare
docker-compose.yaml
Outdated
| minio: | ||
| condition: service_started # used for servering parquet files |
There was a problem hiding this comment.
since python-api service depends on minio, probably ["rest-api"] should be added in profiles of minio service:
minio:
profiles: ["all", "minio", "evaluation-engine", "rest-api"]There was a problem hiding this comment.
Since the service is declared to depend on MinIO, the MinIO service will automatically be started when starting the Python-based API.
I believe that the use of profiles in this docker compose file should be revised. I feel that there are a lot of redundant profiles that make the same mistake. I deliberately left that out of this PR, though.
It could be that the original philisophy was such that you can spin up the services individually in partial states (e.g., REST API without a MinIO backend to serve the datasets). I think that's valid, but even so some of the profiles are confusing to me. It's also worth considering if we should instead provide two services: the base one (e.g., just requiring the rest api + database) and an extended one (that also requires MinIO).
There was a problem hiding this comment.
Revising the profiles sound good! Some background: I added the profiles when setting up the services, to make it easier for myself to test part of the services. I think most users just want to run everything. Only base and extended sounds good to me.
josvandervelde
left a comment
There was a problem hiding this comment.
Looks good to me!
It would be good to make some small additions to the README.md, to add at least the endpoint
Adds the Python-based REST API as a service:
/pypath with nginx. The server itself is unaware of this, but currently does not generate relative links, so there is no harm./data/*). It doesn't require PHP's ES to be up, but I do not believe I can specify that neatly (only with defining a second php-api service, but that doesn't seem worth the effort since the REST API is expected to be able to serve arff files directly (through MinIO) at some point)It might be confusing that
config/pythonnow contains bothopenml-pythonand the server configurations. I guess it would be better to have separate directories?