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

Re-write simple sample app using connexion #295

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Member

commented Mar 7, 2019

Resolves #293

All Submissions:

  • Have you followed the guidelines in our Contributing document?

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

  • Does your PR follow our Code of Conduct?

  • Have you added an explanation of what your changes do and why you'd like us to include them?

  • Does each method or function "do one thing well"? Reviewers may recommend methods be split up for maintainability and testability.

  • Is this code designed to be testable?

  • Is the code documented well?

  • Does your submission pass existing tests (or update existing tests with documentation regarding the change)?

  • Have you added tests to cover your changes?

  • Have you linted your code prior to submission?

  • Have you updated the documentation and README?

  • Is PII treated correctly? In particular, make sure the code is not logging objects or strings that might contain PII (e.g. request headers).

  • Have secrets been stripped before committing?


This change is Reviewable

@c-w c-w requested review from sayar and fnocera Mar 7, 2019

@c-w c-w force-pushed the connexion branch 2 times, most recently from 4392597 to a6f8a53 Mar 7, 2019

@c-w c-w referenced this pull request Mar 7, 2019

Closed

Add argparse to simple app main.py #292

13 of 13 tasks complete

@cicorias cicorias self-requested a review Mar 18, 2019

@cicorias
Copy link
Member

left a comment

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @c-w, @fnocera, and @sayar)


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/Dockerfile.{{cookiecutter.PROJECT_NAME_SLUG}}, line 36 at r1 (raw file):

EXPOSE $PORT

CMD gunicorn --log-level="$LOG_LEVEL" --bind="$HOST:$PORT" --workers="$WORKERS" "main:build_app(wsgi=True)"

should this be the exec form? this is shell form - ie. ['gunicorn', '--log......]

https://docs.docker.com/engine/reference/builder/#cmd

any reason this isn't an ENTRYPOINT?


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/main.py, line 33 at r1 (raw file):

    PORT = getenv('PORT', '8888')
    HOST = getenv('HOST', '127.0.0.1')

any reason dotenv isn't used and these defaults are in settings.py ?


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/requirements.txt, line 1 at r1 (raw file):

connexion[swagger-ui]

imho using swagger is a very opinionated approach for an example application. I don't think it's necessary unless i'm missing something.


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/requirements-dev.txt, line 1 at r1 (raw file):

flake8

have we really reduced all the dependencies that both these requirements.txt files are only used in the sample app?

@cicorias
Copy link
Member

left a comment

#293
again, imho swagger is an opinionated approach.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @c-w, @fnocera, and @sayar)

@c-w
Copy link
Member Author

left a comment

See reply on requirements.txt below.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @cicorias, @fnocera, and @sayar)


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/Dockerfile.{{cookiecutter.PROJECT_NAME_SLUG}}, line 36 at r1 (raw file):

Previously, cicorias (Shawn Cicoria) wrote…

should this be the exec form? this is shell form - ie. ['gunicorn', '--log......]

https://docs.docker.com/engine/reference/builder/#cmd

any reason this isn't an ENTRYPOINT?

The exec form doesn't dereference environment variables.

Exec form:

exec form

Sh form:

sh form

What would be the value of setting gunicorn as the entrypoint instead of the cmd? To my understanding, we want to set the entrypoint if we have multiple valid sets of arguments that could be passed to the container. Here, we really only have one valid set of arguments (running gunicorn) so I'm not sure what setting the entypoint buys us. Could you clarify? One advantage of not setting the entrypoint is that we can more easily exec into the container, e.g. to run bash to debug something. docker run foo cat /etc/hosts is less of a pain to type/remember than docker run --entrypoint=cat /etc/hosts.


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/main.py, line 33 at r1 (raw file):

Previously, cicorias (Shawn Cicoria) wrote…

any reason dotenv isn't used and these defaults are in settings.py ?

We can move the configuration to a different file if you wish. My intent here was to keep the amount of code that the developer has to view to a minimum (it's supposed to be a simple app after all) and contain all the "framework code" in main.py.

Adding dotenv pulls in an additional dependency and we're not currently generating a .env file for the user anyways. Happy to do so, but this was omitted to keep the simple app minimal. Let me know what you prefer.


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/requirements.txt, line 1 at r1 (raw file):

Previously, cicorias (Shawn Cicoria) wrote…

imho using swagger is a very opinionated approach for an example application. I don't think it's necessary unless i'm missing something.

To my understanding, the purpose of this simple application template is to give the developer as little to worry about in terms of framework as possible in order to increase productivity.

Using connexion gives us a lot of stuff for free: routing, request post body validation, serialization, etc. The developer really only has to implement a pure Python function to do the data transformation/model prediction. If we use the previous "standard-library only" approach of making a web server, all of this is code that a developer otherwise would have to read/maintain which is not desirable in my opinion.


agogosml_cli/cli/templates/apps/simple/{{cookiecutter.PROJECT_NAME_SLUG}}/requirements-dev.txt, line 1 at r1 (raw file):

Previously, cicorias (Shawn Cicoria) wrote…

have we really reduced all the dependencies that both these requirements.txt files are only used in the sample app?

Yes we have. For testing, the simple app now uses doctest (built into Python). Pydocstyle and flake8 are the only remaining development dependencies for linting. You can confirm this in the multi-stage dockerfile on lines 16-18.

@c-w c-w requested a review from cicorias Mar 18, 2019

@c-w c-w force-pushed the connexion branch from a6f8a53 to fc23231 Mar 18, 2019

c-w added some commits Mar 18, 2019

@sayar

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

@c-w Let's press pause on this PR and discuss offline (related to upcoming changes to CLI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.