-
Notifications
You must be signed in to change notification settings - Fork 142
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
send some client info to database to track processing issues #198
Conversation
…encap-core into local-docker-dev
check resource usage
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.
@carmichaelong thanks. I left a few comments, only important ones are in docker-compose where dev-specific names/paths should not be merged in prod.
@@ -1,7 +1,7 @@ | |||
version: "3.9" | |||
services: | |||
mobilecap: | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/opencap | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/opencap-dev |
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.
@carmichaelong it should be opencap-dev on dev and opencap on prod. Ideally we find a way to have this not hard coded. Thoughts?
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.
Thanks for the catch. I definitely was too focused on getting this right for dev branch to still work, whoops.. I may have to make another feature branch of this and remove -dev
if we don't want to keep changing that back and forth on this branch.
I agree that we should avoid hard-coding things like this, and I think of it as related to all the different branches that are active and used by different machines (e.g., main vs dev, local vs aws). Been thinking of ways to simplify this and can make a new issue and discuss there!
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.
Sounds good
@@ -16,7 +16,7 @@ services: | |||
count: 1 | |||
capabilities: [gpu] | |||
openpose: | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/openpose | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/openpose-dev |
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.
As above, it should be openpose-dev on dev and openpose on prod.
@@ -27,7 +27,7 @@ services: | |||
count: 1 | |||
capabilities: [gpu] | |||
mmpose: | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/mmpose | |||
image: 660440363484.dkr.ecr.us-west-2.amazonaws.com/opencap/mmpose-dev |
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.
As above, it should be mmpose-dev on dev and mmpose on prod.
LGTM, plus we tested last week. We should address Antoine's feedback and document it. |
@antoinefalisse also adding you as a second reviewer this time to see if @AlbertoCasasOrtiz and I missed something
added:
tested by reprocessing a session on dev (i.e., did not capture new data). happy to do extra testing if needed