irods / irods_capability_automated_ingest

Other
12 stars 15 forks source link

Add demo Compose project for testing #237

Closed alanking closed 1 month ago

alanking commented 3 months ago

No issue number yet...

Derived from the https://github.com/irods/irods_demo Compose project.

I used this to confirm S3 scans (no automated tests yet). Not sure if we want to include this in the project but I'm putting it up just in case anyway.

alanking commented 1 month ago

I have found this addition to be very helpful in quickly iterating for some new features. One can copy files into the container and restart the ingest worker service to apply changes very easily, and this works much better than the existing Docker stuff in this repo.

korydraughn commented 1 month ago

Good. Is it worth documenting that development approach in the README for others who are interested in modifying the code?

alanking commented 1 month ago

Possibly. It may (or may not) be better to use volume mounts to install the package rather than building it into the image as is being done now. This would allow the developer to make changes in the host machine and changes would be applied to all instances of the ingest worker service rather than having to copy the files into each container. This would of course be an alternative approach, and may have some drawbacks from the current implementation. I don't think we should go for that right away.

Anyways, we can add a section to the README with a "suggested workflow" for developers. As is implied above, there are probably better ways to do this, so I'm a little hesitant to say how one should use this thing.

korydraughn commented 1 month ago

That's reasonable. We can always add words to the README or somewhere else if it proves to be helpful.

What's left for this PR?

alanking commented 1 month ago

I think I need to adjust the default number fed to the -c option for Celery. Using the default - which matches the number of cores on the host - causes the iRODS server to become unusable for modestly large ingest jobs, so it would be better to reduce this to, like, 2 or even 1. This would probably involve adding a CMD line which can be overridden in the Compose YAML file or using the command line options.

alanking commented 1 month ago

Well... I suppose I can mark this as ready for review now

trel commented 1 month ago

ha. i think squash em. certainly good enough to get in - no functional changes to the software directly.

alanking commented 1 month ago

Had one last thing regarding the Celery concurrency adjustments. I tried doing this with command line options but Compose does some weird stuff (creates containers with a -run-<sha> suffix) when using docker compose run and the like, so it's probably just best to make adjustments before the containers are created for the service instances. A note saying as much has been added to the README and the YAML file.

alanking commented 1 month ago

Okay, squashed and given issue #244.

korydraughn commented 1 month ago

I think we're ready for squashing.

alanking commented 1 month ago

Squashed

alanking commented 1 month ago

'd. Mergin