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.

Sign up for GitHub

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

Jump to bottom

add docker containers to training #1071

Merged
merged 11 commits into from
Dec 18, 2023
Merged

add docker containers to training #1071

merged 11 commits into from
Dec 18, 2023

Conversation

andrewpeng02
Copy link
Contributor

@andrewpeng02 andrewpeng02 commented Dec 6, 2023 •
edited by dwu359
Loading

Dockerize the /training route backend

What user problem are we solving?
In order to eventually deploy and easily scale the /training endpoints, we want to put it in docker containers.

What solution does this PR provide?
I created both a dev and prod docker containers that can be created with docker-compose

Testing
https://youtu.be/TdtIas9IEbU

closes #911

@sweep-ai Sweep AI
Copy link
Contributor

sweep-ai bot commented Dec 6, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

dwu359
dwu359 reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@dwu359 dwu359 left a comment

Choose a reason for hiding this comment

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

Good work so far, keep it up

training/nginx/Dockerfile Outdated Show resolved Hide resolved
training/Dockerfile.prod Outdated Show resolved Hide resolved
@andrewpeng02 andrewpeng02 marked this pull request as ready for review December 7, 2023 16:50
@andrewpeng02 andrewpeng02 requested a review from a team as a code owner December 7, 2023 16:50
@karkir0003
Copy link
Member

@andrewpeng02 can you add a test plan or screen recording to test that the dockerfile works?

@karkir0003
Copy link
Member

looks good to me. put two minor questions

@karkir0003
Copy link
Member

@dwu359 can we link a github issue (if any) to this PR?

dwu359
dwu359 reviewed Dec 8, 2023
View reviewed changes
Copy link
Contributor

@dwu359 dwu359 left a comment

Choose a reason for hiding this comment

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

left some more comments

training/Dockerfile Outdated Show resolved Hide resolved
training/Dockerfile Show resolved Hide resolved
karkir0003
karkir0003 reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@karkir0003 karkir0003 left a comment

Choose a reason for hiding this comment

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

added two comments @andrewpeng02

training/docker-compose.prod.yml Outdated Show resolved Hide resolved
training/docker-compose.yml Outdated Show resolved Hide resolved
@karkir0003
Copy link
Member

@andrewpeng02 can we update readme on how to run the dockerfiles?

karkir0003
karkir0003 approved these changes Dec 9, 2023
View reviewed changes
Copy link
Member

@karkir0003 karkir0003 left a comment

Choose a reason for hiding this comment

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

looks good

dwu359
dwu359 reviewed Dec 10, 2023
View reviewed changes
Copy link
Contributor

@dwu359 dwu359 left a comment

Choose a reason for hiding this comment

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

image
I'm running into this error when running your code and I have trouble trying to find the root cause of it, do you happen to know what's causing this?

training/Dockerfile Outdated Show resolved Hide resolved
training/Dockerfile Outdated Show resolved Hide resolved
@karkir0003
Copy link
Member

@dwu359 what cmd did u run?

@andrewpeng02

@dwu359
Copy link
Contributor

dwu359 commented Dec 11, 2023

@dwu359 what cmd did u run?

@andrewpeng02

docker-compose up --build

@karkir0003
Copy link
Member

@dwu359 what cmd did u run?
@andrewpeng02

docker-compose up --build

run within training/ directory?

dwu359
dwu359 reviewed Dec 12, 2023
View reviewed changes
training/Dockerfile Outdated Show resolved Hide resolved
@karkir0003
Copy link
Member

@andrewpeng02 docker image works on my end with dev mode

@karkir0003
Copy link
Member

@andrewpeng02 some build checks failed

sonarcloud passed

@karkir0003
Copy link
Member

@andrewpeng02 u should be unblocked!!

@karkir0003
Copy link
Member

@dwu359 i approved on my end. can @andrewpeng02 merge?

@andrewpeng02
Copy link
Contributor Author

I need to push a quick fix first

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 18, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@andrewpeng02 andrewpeng02 added this pull request to the merge queue Dec 18, 2023
Merged via the queue into nextjs with commit 0c61b5d Dec 18, 2023
4 checks passed
@andrewpeng02 andrewpeng02 deleted the dockerize-backend branch December 18, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwu359 dwu359 dwu359 left review comments

@farisdurrani farisdurrani farisdurrani approved these changes

@karkir0003 karkir0003 karkir0003 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Use Gunicorn to run training backend in prod
4 participants
@andrewpeng02 @karkir0003 @dwu359 @farisdurrani