-
Notifications
You must be signed in to change notification settings - Fork 35
Add docker support for local development #2878
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
base: master
Are you sure you want to change the base?
Conversation
| # Start the API | ||
| gunicorn -b :"$PORT" policyengine_api.api --timeout 300 --workers 5 --preload & | ||
|
|
||
| # Start multiple workers using POSIX-compliant loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the README.md and ./gcp/policyengine_api/start.sh mention worker.py but I found no file with that name in the repo. Are the docs just out of date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the docs are out of date. We instead delegate to an external service for what we used to use these workers for.
|
Thank you @pkarman! @anth-volk will take a look when he's back from vacation next week. |
anth-volk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this @pkarman and apologies for the delay in review. I left one concern around how the API currently interfaces with an external API service, but otherwise, this would be amazing to have. Have you built/run this locally at all?
| # Start the API | ||
| gunicorn -b :"$PORT" policyengine_api.api --timeout 300 --workers 5 --preload & | ||
|
|
||
| # Start multiple workers using POSIX-compliant loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the docs are out of date. We instead delegate to an external service for what we used to use these workers for.
| "GOOGLE_APPLICATION_CREDENTIALS not set; unable to run simulation API." | ||
| ) | ||
|
|
||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue, unclear impact: We call an external service using this GAC; without it, the app will not return a society-wide simulation result. I'm unclear on how we might handle this; do you have any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the issue here is along the lines of "how should the app behave if it is missing an expected configuration?"
Currently the code both logs an error and raises an exception during app start-up time, as the SimulationAPI service checks for the config when it is instantiated. Maybe the config check could be moved to a new setup() method and deferred until the first time run() is called? That way the rest of the API could start without relying on a (possibly non-existent) GAC config.
In my case, I was using the MyFriendBen benefits API against a local instance of the PolicyEngineAPI and I didn't need the full society-wide simulation service at all.
So if the config check were somehow deferred till it was actually needed, that would be ideal from my pov.
Perhaps __init__ could continue to check for the config, and maybe warn instead of error log when missing, but the actual exception could be moved till run() time.
| logger = logging.getLogger(__name__) | ||
|
|
||
| # shims to make default logger act like google's | ||
| levels = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accolade: Like the shims!
Yes, I have run built and run locally, in coordination with this PR for MyFriendBen. If you approve of my suggestion for deferring the GAC check, I'd be happy to implement that as part of this PR. |
|
Just to check with you @pkarman, my understanding had been that MyFriendBen was using our household API, which already has some Docker functionality, though no Docker compose. This package here is the API we use in production to host our website. Were you looking to move toward using our full API, as well? |
Purpose
These changes add Docker support to allow for local development consistent with the deployed environment. In theory, this would allow for deployment into alternate cloud platforms.
Changes
MakefilemaketargetsSee also
MyFriendBen/benefits-api#1216