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

fix indexing startup for devcontainer. #1563

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

juanmrad
Copy link
Collaborator

Summary

Fixes #1507

This PR adds a check on the app config to ensure we do not run background tasks when we are just running the DB migrations. That way we do not have race conditions where we are expecting DB to have data when the data is still not migrated.

Test Plan

Tested locally by cleaning up my DB and started the containers again having TASK_INDEX_CACHE set to true and did not see any errors. while it failed when I reverted my change and cleaned up my db.

I tried to add a CI check for ensuring no model changes are uncommitted with:

  check-pending-migrations:
    name: Check Pending Migrations
    runs-on: ubuntu-latest
    # Service containers to run with `tests`
    services:
      postgres:
        image: postgres
        # Provide the env variables
        env:
          POSTGRES_DB: media_match_test
          POSTGRES_USER: media_match
          POSTGRES_PASSWORD: hunter2
        ports:
          - 5432:5432
        # Set health checks to wait until postgres has started
        options: >-
          --health-cmd pg_isready
          --health-interval 10s
          --health-timeout 5s
          --health-retries 5
    steps:
      - uses: actions/checkout@v2
      - name: Build docker image
        run: docker build -t local .
      - name: Test with pytest on docker image
        run: |
          docker run --network="host" local sh -c "python -m pip install --upgrade pip ;
            pip install -e '.[test]' ;
            MIGRATION_COMMAND=1 flask --app OpenMediaMatch.app db check --directory /workspace/src/OpenMediaMatch/migrations"

But this may need more work, and be best for a separate PR.

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

This environment variable solution looks like it might work, but I wonder if we want to generalize it even more and only start apscheduler when the run command has been triggered.

I think triggering these during app creation and not during pre-serve/pre-fork might have been a design mistake.

@@ -33,7 +33,7 @@
# APScheduler (background threads for development)
TASK_FETCHER = True
TASK_INDEXER = True
TASK_INDEX_CACHE = False # Needs a fix to allow initial startup
TASK_INDEX_CACHE = True # Needs a fix to allow initial startup
Copy link
Contributor

Choose a reason for hiding this comment

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

If you fixed it, remove the comment :P

@@ -1,5 +1,5 @@
#!/bin/bash
set -e
export OMM_CONFIG=/workspace/.devcontainer/omm_config.py
flask --app OpenMediaMatch.app db upgrade --directory /workspace/src/OpenMediaMatch/migrations
MIGRATION_COMMAND=1 flask --app OpenMediaMatch.app db upgrade --directory /workspace/src/OpenMediaMatch/migrations
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking question: As an alternative, can we detect whether the "Run" command has been triggered, and only startup apscheduler then?

# this is the Alembic Config object, which provides
# access to the values within the .ini file in use.
# It's also impossible to type!
Copy link
Contributor

Choose a reason for hiding this comment

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

Guess I was wrong :P

@@ -248,11 +248,7 @@ def as_storage_iface_cls_typed(
],
) -> TCollabConfig:
cls = exchange_cls.get_config_cls()
# This looks like it should be typed correctly, but too complicated
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you know, it was typed correctly :P

@Dcallies Dcallies merged commit e7bf6aa into facebook:main Mar 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

psycopg2.errors.UndefinedTable: relation "signal_type_override" does not exist
3 participants