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

(Fixes #208)Add MongoDB integration persister #215

Merged
merged 5 commits into from
May 30, 2024

Conversation

NandaniThakur
Copy link
Contributor

This PR adds a new MongoDB persister to the project, allowing state data to be saved and loaded from a MongoDB database. The implementation follows the existing structure of the Redis persister for consistency.
Fixes #208.

Changes

  1. Added MongoDBPersister class to handle saving and loading state data to/from MongoDB.
  2. Implemented methods: save, load, list_app_ids, and helper method create_key.
  3. Added test_mongodb_persister.py to test the functionality of MongoDBPersister.

How I tested this

Created unit tests in test_mongodb_persister.py to verify the correct functionality of save, load, and list_app_ids methods.
Mocked MongoDB interactions to ensure tests do not depend on a live MongoDB instance.

Notes

Updated created_at to use timezone-aware datetime objects.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Excellent thanks! Just a few comments to fix up.

One more:

  • please add then a reference to the class in persisters.rst under docs/.

burr/integrations/persisters/b_mongodb.py Outdated Show resolved Hide resolved
burr/integrations/persisters/b_mongodb.py Outdated Show resolved Hide resolved
burr/integrations/persisters/test_mongodb_persister.py Outdated Show resolved Hide resolved
@NandaniThakur
Copy link
Contributor Author

If there is something which I missed or anything. please let me know.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. A few details.

To fix the pre-commit test failure, you'll need to install the pre-commit hook.

pip install pre-commit
pre-commit install
pre-commit run --all-files

docs/reference/persister.rst Outdated Show resolved Hide resolved
burr/integrations/persisters/b_mongodb.py Outdated Show resolved Hide resolved
burr/integrations/persisters/test_mongodb_persister.py Outdated Show resolved Hide resolved
burr/integrations/persisters/b_mongodb.py Outdated Show resolved Hide resolved
@@ -1 +1 @@
../examples
Copy link
Contributor

Choose a reason for hiding this comment

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

did you change a symlink here? or?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you revert this please.

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 ran git ls-tree -l HEAD and verified the repository structure. It appears that the examples symlink was not intentionally modified. My recent changes were limited to updating b_mongodb.py, persisters.rst, pyproject.toml, and persisters.yml.

Comment on lines 1 to 34
import unittest

from burr.core import state
from burr.integrations.persisters.b_mongodb import MongoDBPersister


class TestMongoDBPersister(unittest.TestCase):
def setUp(self):
self.persister = MongoDBPersister(
uri="mongodb://localhost:27017", db_name="testdb", collection_name="testcollection"
)

def tearDown(self):
self.persister.collection.drop()

def test_save_and_load_state(self):
self.persister.save("pk", "app_id", 1, "pos", state.State({"a": 1, "b": 2}), "completed")
data = self.persister.load("pk", "app_id", 1)
self.assertEqual(data["state"].get_all(), {"a": 1, "b": 2})

def test_list_app_ids(self):
self.persister.save("pk", "app_id1", 1, "pos1", state.State({"a": 1}), "completed")
self.persister.save("pk", "app_id2", 2, "pos2", state.State({"b": 2}), "completed")
app_ids = self.persister.list_app_ids("pk")
self.assertIn("app_id1", app_ids)
self.assertIn("app_id2", app_ids)

def test_load_nonexistent_key(self):
state_data = self.persister.load("pk", "nonexistent_key")
self.assertIsNone(state_data)


if __name__ == "__main__":
unittest.main()
Copy link
Contributor

@skrawcz skrawcz May 28, 2024

Choose a reason for hiding this comment

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

Sorry, I apologize for missing this earlier, but we use pytest and not unittest.

This shouldn't be a big change to get it to work. You'll just need to use a pytest fixture for the persister. Do you need more pointers here, or can you handle this yourself?

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 dont know weather i did it as required. help me if it is not correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Close - can you rename the file to test_b_mongodb.py please? that's the pytest convention. test_NAME_OF_MODULE.py.

Otherwise we will need to set up the test environment to install mongodb for the test to actually pass.

Why don't we try adjusting part of python-package.yml in .github/workflows to be something like this:

  test:
    runs-on: ubuntu-latest
    strategy:
      fail-fast: false
      matrix:
        python-version: ['3.9', '3.10', '3.11', '3.12']

    steps:
      - uses: actions/checkout@v3
      - name: Set up Python ${{ matrix.python-version }}
        uses: actions/setup-python@v3
        with:
          python-version: ${{ matrix.python-version }}
      - name: Install dependencies
        run: |
          python -m pip install -e ".[tests,tracking-client]"
      - name: Run tests
        run: |
          python -m pytest tests --ignore=tests/integrations/persisters
  test-persister-dbs:
    runs-on: ubuntu-latest
    services:
      mongodb:
        image: mongo:7.0
        ports:
          - 27017:27017
      postgres:
        image: postgres:15
        ports:
          - 5432:5432
      redis:
        image: redis:7
        ports:
          - 6379:6379
    strategy:
      fail-fast: false
      matrix:
        python-version: ['3.9', '3.10', '3.11', '3.12']

    steps:
      - uses: actions/checkout@v3
      - name: Set up Python ${{ matrix.python-version }}
        uses: actions/setup-python@v3
        with:
          python-version: ${{ matrix.python-version }}
      - name: Install dependencies
        run: |
          python -m pip install -e ".[tests,tracking-client]"
      - name: Run tests
        run: |
          python -m pytest tests/integrations/persisters/

Can you add that please?

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 did that, please let me know whether it was correct or not.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes! You've made the project better with these additions, thanks!

@skrawcz
Copy link
Contributor

skrawcz commented May 29, 2024

I'll merge this tomorrow my time -- thanks @NandaniThakur !

@skrawcz skrawcz mentioned this pull request May 29, 2024
14 tasks
@NandaniThakur
Copy link
Contributor Author

NandaniThakur commented May 29, 2024

I'll merge this tomorrow my time -- thanks @NandaniThakur !

As this was my first open source, It was quite fun & exciting to do. thank you for your guidance and help in completing this.
I'll look forward to the merging of PR. Thanks @skrawcz !!!

@skrawcz skrawcz merged commit d0d9d84 into DAGWorks-Inc:main May 30, 2024
11 checks passed
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.

MongoDB integration
2 participants