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

[APP-3167] Add flask app based on the code from slack bot repo #1

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

kideh88
Copy link
Collaborator

@kideh88 kideh88 commented Oct 15, 2024

Add simple Flask app with Hello world sample.

The github files are copies of the other OSS templates, no changes have been made

Files of significance:

  • flask_app.py
  • start-app.sh
  • README.md

Copy link
Contributor

@ben-cutler-datarobot ben-cutler-datarobot 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 have a few pieces of feedback on wording / thoughts.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/SECURITY.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
```

## How to add and use runtime parameters?
Create a metadata.yaml file in your application source folder. Here is an example of a DEPLOYMENT_ID:
Copy link
Contributor

Choose a reason for hiding this comment

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

The MLOPS_RUNTIME_PARAM_ suffix is really confusing, and I think we should be really loud about its existance

Suggested change
Create a metadata.yaml file in your application source folder. Here is an example of a DEPLOYMENT_ID:
Create a metadata.yaml file in your application source folder. Here is an example of a DEPLOYMENT_ID which will create an environment variable called `MLOPS_RUNTIME_PARAM_DEPLOYMENT_ID`:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree! Will add this text fix to the other OSS repos too 👍

In the `./src/templates` directory you will find a sample index page. You can add additional HTML templates here, and
call them by adding new routes in the `flask_app.py` like this:
```python
@flask_app.route("/new-page")
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this route need to be /apps/<app-id>/new-page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this piece from Chrsitians template code , where he used a proxy fix:
flask_app.wsgi_app = ProxyFix(flask_app.wsgi_app, x_prefix=1)

https://werkzeug.palletsprojects.com/en/2.3.x/middleware/proxy_fix/

src/start-app.sh Outdated
Comment on lines 6 to 7

gunicorn -b :8080 flask_app:flask_app --timeout 200 --graceful-timeout 30 &
Copy link
Contributor

Choose a reason for hiding this comment

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

Does running via gunicorn automatically apply the prefix 👀 👀 👀 👀 👀 👀

Copy link
Collaborator Author

@kideh88 kideh88 Oct 16, 2024

Choose a reason for hiding this comment

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

ProxyFix (previous comment) will do that, gunicorn gives us a production level server - the one that comes with Flask is only recommended for development, not prod

@kideh88 kideh88 merged commit f46e69e into main Oct 16, 2024
@kideh88 kideh88 deleted the kimd/APP-3167-flaskapp branch November 6, 2024 14:13
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