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 LazyInit class to only init when getting attribute of pymongo.MongoClient #59

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

dachengx
Copy link

@dachengx dachengx commented Oct 4, 2024

Close #58

The LazyInit will only initialize the instance only if the attribute or method of pymongo.MongoClient like find.

Copy link
Member

@cfuselli cfuselli left a comment

Choose a reason for hiding this comment

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

Hey,

I think these changes make sense in general, but unfortunately I lack some knowledge on admix to understand when and where these functions are used, so please do not take my comments too seriously.

But, here are some questions:

  • why is it needed to initialise the functions in the utils.py at all? Why not just call the utilix function directly in the donwloader.py? it seems like a not needed duplication to me..
  • just wanted to stress that those are 1t functions, took me a while to notice that ;)

@dachengx
Copy link
Author

dachengx commented Oct 9, 2024

re these functions are used, so please do not take my comments too seriously.

I guess initially these instances are initialized in utils.py so that we do not need to make the connection in each function.

How to optimize this depends on how often these functions are called. I do not have enough knowledge either. This PR is just an expedient that can lower the DB connection.

@dachengx dachengx requested a review from cfuselli October 10, 2024 18:11
@lucascottolavina
Copy link
Contributor

I had a look only now the code and it seems it does well its job. I validate it now, but before merring I suggest for a sake of safety, to also try it on a real scenario with a stress test to verify that it does what it promises to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants