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

WIP: Displaying a way to break the chain_db when mongo-only collections are present #799

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

zthatch
Copy link
Contributor

@zthatch zthatch commented Jul 14, 2021

Breaks the ability to chain across mongo and filesystem.

Power shown in a_proprevhelper.py, which realizes that we have a mongo collection and decides not to load all documents.

Main changes in client_manager.py (allows you to still load all from collections if you so choose), mongoclient line 380, and database.xsh.

Commented code in chain_db is potentially exciting because an update to getitem (not there yet) could allow the typical chaining method to work by running finds and chaining their results at get-time (all documents time), but also allow helper maintainers to check and see if their collection is purely mongo-backed, and if so run mongo commands.

@zthatch zthatch marked this pull request as draft July 14, 2021 19:04
@zthatch
Copy link
Contributor Author

zthatch commented Jul 14, 2021

@sbillinge this is to show how I am playing with the chaining. Details in above comment.

(most file changes are from an inconsequential change, above comment points to consequential changes)

@sbillinge
Copy link
Contributor

sbillinge commented Jul 16, 2021 via email

…t doesn't break tests, but does remove ability to chain across mongo and fs. Commented code in chain_db en route to fix this.
… on each of the milestones, deliverables, and kickoffs before the updater runs at all. Likely because of a negligence to implement a deepcopy method in the chained collections leading to issues after the milestones have been listed.
…base.xsh to put collections into their respective maps rather than the outputs of the iter method.
@zthatch
Copy link
Contributor Author

zthatch commented Jul 22, 2021

Refactored in such a way that we can now do everything we have ever wanted with minimal transitions.

The databases used to chain at only the document level, which made it impossible to use the chained database as anything other than a reference when attempting to chain across the fs and mongo.

This has been changed such that there is now a chaining object at the collection level as well, which makes a distinction between mongo and fs collections and handles the chaining in a way that allows the helper maintainers to check for a fs components, and if it is not present they can use mongo operations.

This was done in such a way that we can maintain the original functionality of chaining across mongo and filesystem collections, contrary to the previous attempt.

Test failures are not happening on windows, resolving on VM.

@sbillinge
Copy link
Contributor

wow, this is exciting news!

…d it must load all of the mongo collections into memory.
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #799 (5d3017e) into master (00b30dd) will decrease coverage by 0.17%.
The diff coverage is 74.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #799      +/-   ##
==========================================
- Coverage   72.17%   72.00%   -0.18%     
==========================================
  Files          71       71              
  Lines        7501     7526      +25     
==========================================
+ Hits         5414     5419       +5     
- Misses       2087     2107      +20     
Impacted Files Coverage Δ
regolith/builders/activitylogbuilder.py 89.32% <ø> (-0.06%) ⬇️
regolith/builders/appraisalbuilder.py 0.00% <ø> (ø)
regolith/builders/cpbuilder.py 92.85% <ø> (-0.11%) ⬇️
regolith/builders/cvbuilder.py 98.48% <ø> (-0.03%) ⬇️
regolith/builders/htmlbuilder.py 96.87% <ø> (-0.04%) ⬇️
regolith/builders/internalhtmlbuilder.py 62.58% <ø> (-0.27%) ⬇️
regolith/builders/manuscriptreviewbuilder.py 100.00% <ø> (ø)
regolith/builders/postdocadbuilder.py 25.00% <ø> (-2.59%) ⬇️
regolith/builders/preslistbuilder.py 95.23% <ø> (-0.12%) ⬇️
regolith/builders/proposalreviewbuilder.py 95.00% <ø> (-0.13%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00b30dd...5d3017e. Read the comment docs.

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.

2 participants