voxel51 / fiftyone-teams-app-deploy

Deployment Assets for the Fiftyone Teams App
Apache License 2.0
3 stars 1 forks source link

Adding workers to docker compose to be used for executing delegated operations #207

Open CamronStaley opened 4 weeks ago

CamronStaley commented 4 weeks ago

Rationale

As a new initiative we want users to be able to run delegated operations out of the box without airflow. These changes will allow users to configure X amount of workers which will pull delegated operations at a set interval and run them in the background on another container.

Changes

Added workers to the plugin compose files with similar configuration to plugins except additional env variables:

Checklist

via https://github.com/voxel51/fiftyone-teams-app-deploy/pull/210

Testing

  1. Pushed executor changes to this PR / Branch: https://github.com/voxel51/fiftyone-teams/pull/865/files
  2. Built image with those changes: https://github.com/voxel51/cloud-build-and-deploy/actions/runs/11244225627
  3. SSH'd into voxel51-fiftyone
  4. Navigated to eric.dev.fiftyone.ai compose files: cd ../../teams/deploy/eric/fiftyone-teams-app-deploy/docker/internal-auth
  5. Copied the contents of these files into the compose files backing that url and ran: docker compose -f compose.dedicated-plugins.yaml -f compose.override.scheduler.testing.yaml up -d
  6. Verified DOs work in Eric's env: https://eric.dev.fiftyone.ai/datasets/quickstart/runs

Logs from container:

camronstaley@voxel51-fiftyone:/teams/deploy/eric/fiftyone-teams-app-deploy/docker/internal-auth$ docker logs eric-teams-delegated-operator-worker-1
Executor started
Executing operation 6705b764e340919775656060
Computing metadata...
 100% |███████| 200/200 [2.3s elapsed, 0s remaining, 87.7 samples/s]
findtopher commented 4 weeks ago

This should target a release branch, not main, so that it's not rolled out to customers before the release is published.

swheaton commented 4 weeks ago

Nice work so far Cam. Thanks topher! Yeah this should be possible with either dedicated or internal plug-in mode. I guess builtin plug-in could use it but it would be pretty useless since we currently have no delegated builtin operators

swheaton commented 4 weeks ago

Additionally, this is agnostic of legacy auth vs. internal auth, so should be available for both. Whether that means having a separate override file that layers it on, or adding to all the existing compose files - I don't know the best practices around that, will have to rely on Aloha

CamronStaley commented 4 weeks ago

@findtopher I could see an argument for either. Workers will only be used if the user is using one of the plugin compose files, but it doesn't mean they have to use them (hence why I added the replicas option defaulting to 0). Would really just change the instructions to the user in the readme and reduce some repetition in the files so it's up to yall.

findtopher commented 4 weeks ago

Additionally, this is agnostic of legacy auth vs. internal auth, so should be available for both. Whether that means having a separate override file that layers it on, or adding to all the existing compose files - I don't know the best practices around that, will have to rely on Aloha

Currently that'll mean adding a new compose.delegated-operators.yaml to both legacy-auth and internal-auth - you can put all the duplicated items in docker/common-services.yaml, so compose.delegated-operators.yaml might be as simple as:

services:
  teams-do:
    extends:
      file: ../common-services.yaml
      service: teams-do-common
findtopher commented 4 weeks ago

@findtopher I could see an argument for either. Workers will only be used if the user is using one of the plugin compose files, but it doesn't mean they have to use them (hence why I added the replicas option defaulting to 0). Would really just change the instructions to the user in the readme and reduce some repetition in the files so it's up to yall.

@CamronStaley - I would put the configs in the common services file, then add a new compose file that extends that service.

If people want delegated operators they would run:

docker compose \
  -f compose.plugins.yaml \
  -f compose.dedicated-operators.yaml \
  -f compose.override.yaml \
  up -d

That provides the most flexibility, and least redundant entries...

swheaton commented 4 weeks ago

Sweet. Should we call it teams-do? Sure would be nice to have a short and sweet service name. But nobody knows what "do" means, unlike "app" and "api".

findtopher commented 4 weeks ago

Sweet. Should we call it teams-do? Sure would be nice to have a short and sweet service name. But nobody knows what "do" means, unlike "app" and "api".

if it was in it's own compose.delegated-operators.yaml file, teams-do might make more sense Or you could probably just make a comment in the plugins yaml files you've updated I would call it teams-do but I carry a lot more cognitive load than lots of folks seem to want to 🤷

teams-dops teams-oper teams-delo

teams-delegated-operator-worker does seem like a mouthful, particularly when you add the name of the compose stack in front of it (minio-teams-delegated-operator-worker)

CamronStaley commented 4 weeks ago

I like teams-do I updated it

swheaton commented 3 weeks ago

@CamronStaley we will need to add tests to tests/integration/compose and tests/unit/compose.

swheaton commented 3 weeks ago

Intg tests will not pass because code from develop>v2.1 is required but tag 2.1 is used for these tests. We can wait until dev-cut, or fudge it with a develop tag in this branch? Not sure how this has been done in past releases.

However, they are failing for a different reason currently. @CamronStaley I think you forgot to git add the new compose.override files you added.

    darwinOverrideFileDO      = "../../tests/fixtures/docker/compose.override.darwin_do.yaml"
...
    mongodbComposeFileDO      = "../../tests/fixtures/docker/compose.override.mongodb_do.yaml"
CamronStaley commented 3 weeks ago

@swheaton yeah that's weird the .gitignore file had "compose.override*yaml" in it for some reason which was causing them not to be picked up. I just assumed I commited them because they weren't showing up in git status

swheaton commented 3 weeks ago

Probably so that any local testing files don't get committed