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

[Debug] Adding wrappers around Mephisto's sqlite3 DB access #911

Closed
wants to merge 4 commits into from

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Oct 4, 2022

Overview

Wraps the creation of sqlite3 Connection objects such that, if any thread tries to access them from a different thread, we'll see this behavior occur. This covers all of the sqlite3 connections inside of Mephisto, so @mojtaba-komeili if you still encounter the bug but aren't receiving additional logging, we'll need to look into your task-specific code.

At the moment this is only a debug branch, and we may choose to remove later.
NOTE - it's possible that Mephisto is silently swallowing an error somewhere, but the debug logging here should point that out as well.

Testing

Automatic tests - currently failing. Will debug

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2022
@JackUrb JackUrb changed the title Adding wrappers around Mephisto's sqlite3 DB access [Debug] Adding wrappers around Mephisto's sqlite3 DB access Oct 4, 2022
@JackUrb JackUrb removed the request for review from mojtaba-komeili October 4, 2022 21:50
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2022

Codecov Report

Base: 64.61% // Head: 64.65% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (85553d8) compared to base (fdf633d).
Patch coverage: 80.64% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #911      +/-   ##
==========================================
+ Coverage   64.61%   64.65%   +0.04%     
==========================================
  Files         108      108              
  Lines        9329     9357      +28     
==========================================
+ Hits         6028     6050      +22     
- Misses       3301     3307       +6     
Impacted Files Coverage Δ
mephisto/abstractions/databases/local_database.py 89.17% <79.31%> (-0.64%) ⬇️
...to/abstractions/providers/mturk/mturk_datastore.py 61.48% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mojtaba-komeili
Copy link
Contributor

Thanks for implementing it. I will start my next task run on this branch. Hopefully this solves it, this time.

@JackUrb JackUrb closed this Dec 9, 2022
@JackUrb JackUrb deleted the wrap-connection-for-thread-check branch December 9, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants