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

Add docker file for dependencies #66

Merged
merged 14 commits into from
Sep 27, 2024
Merged

Add docker file for dependencies #66

merged 14 commits into from
Sep 27, 2024

Conversation

russelldj
Copy link
Contributor

This is a step toward running containerized workflows. This file seems to work, but Metashape is not activated. For internal use, I expect that there is a way to set it up to communicate with the license server, but I'm not sure what that involves. @youngdjn, any thoughts?

@russelldj
Copy link
Contributor Author

To run this container, run sudo docker run -v </dir/with/images>:/data --gpus all -e AGISOFT_FLS=$AGISOFT_FLS -i -t test:latest /bin/bash. I'll add this to the documentation somewhere with more explanation but wanted to get it recorded for the time being.

@youngdjn
Copy link
Collaborator

Metashape version is up to 2.1.3 and that's what we're using for new ofo dev VMs through CACAO (oh actually 2.1.2 currently but I should update it). Are you intentionally using v2.1.1 for the docker image?

@russelldj
Copy link
Contributor Author

Nope, sorry, just copied from Jeff's image. I'll update it.

Also, can I try to follow the same steps you did for the toy workflow to set up the github container registry?

@youngdjn
Copy link
Collaborator

Awesome. Yes please do! I believe you'll just need the same github action (possibly tweaked depending on how you want the image tagged), and potentially one repo-level setting to enable packages (I can't remember).

@russelldj russelldj force-pushed the feature/DR/add-docker-file branch from 6b7c8b8 to 2d286be Compare September 26, 2024 19:25
@russelldj russelldj changed the title WIP: Add docker file for dependencies Add docker file for dependencies Sep 27, 2024
@russelldj
Copy link
Contributor Author

I think this is good to go. The only change I made to the actual python code was to replace the existing command line parsing with functionality from the argparse module. This was because sys.stdin.isatty() never evaluated to true within docker. The one minor inconvenience is the output data is owned by the root user. This seems like a known headache of docker that I'd prefer to address as a separate issue.

@russelldj russelldj requested a review from youngdjn September 27, 2024 15:26
@russelldj
Copy link
Contributor Author

While running a new dataset I got a MemoryError: std::bad_alloc error on the orthomosaic stage. I was on a small GPU instance so I suspect that this was just a simple out of memory error. But I'll try again with a bigger instance to make sure.

Copy link
Collaborator

@youngdjn youngdjn 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 only have a few suggested additions to the documentation.

After merging, we should also discuss how we want to schedule container image pushes...e.g. possibly also from branches to facilitate testing during development?

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines +24 to +31
def parse_args():
parser = argparse.ArgumentParser()
parser.add_argument("config_file", default=manual_config_file)

args = parser.parse_args()
return args

args = parse_args()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is smart and much nicer. Thanks!

```
docker run -v </host/data/dir>:/data -e AGISOFT_FLS=$AGISOFT_FLS --gpus all ghcr.io/open-forest-observatory/automate-metashape
```
Note that the owner of the output data will be the `root` user. To set the ownership to your user account, you can run `sudo chown <username>:<username> <file name>` or `sudo chown <username>:<username> -R <folder name>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add above this line (or elsewhere if you find a better spot):

If running Docker on Linux without sudo (as in this example), your user will need to be in the docker group. This can be achieved with sudo usermod -a -G docker $USER and then logging out and in, as explained here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the top since it feels like another docker setup step.

@russelldj
Copy link
Contributor Author

@youngdjn I think this should address all your feedback. I'm just trying to figure make sure I have the workflow_dispatch command right. But feel free to merge before that if you want.

@russelldj
Copy link
Contributor Author

The syntax seemed a little wonky but it follows the example from geograypher here. I think this is ready to merge.

@youngdjn youngdjn merged commit e63e7b4 into main Sep 27, 2024
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