-
Notifications
You must be signed in to change notification settings - Fork 31
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
Massive refactoring to get ready for v1.0 #62
Conversation
Just coming back online so only just seen this -- this is one big PR! Difficult to read the diff since it seems like a lot of code has moved, what would you say are the biggest changes for someone wanting to write or run a builder? I saw mrun is gone also and there's a new cli. Other questions:
|
No worries. Realized this was such a large change that it wasn't appropriate for a review. This is mostly moving stuff around to have a coherent structure and adding in all the things we've been missing: tests, docstrings, and type annotations. Stores that have seen real-world use:
Yes, I still want to add in build indicators, but I'm not yet sure how to do that. The big issue is how to make this generic so everyone doesn't have to implement something. |
|
Ok, got it, thanks. What's the distinction between JointStore and ConcatStore? Feels like both might be prone to bugs. I remember JointStore came about for specific performance reasons, can't remember where it's used though. Re. build indicators/reporting, my idea was to add an extra kwarg to the base Builder class, say I don't know that that's the best idea, but it does avoid requiring the user writing any custom code in a specific builder. |
|
It might make sense to drop |
I’m fine with that, but we’d still need an interface to the Views via
Python though? Cleanest way to do this without our current system may be to
modify JointStore to take a list of Stores as inputs and then modify the
get_pipeline function to retrieve the appropriate View?
I’m not intending to use this much myself though, I don’t see it as a big
priority. As long as what we have works.
…On Thu, Jan 2, 2020 at 10:34, Shyam Dwaraknath ***@***.***> wrote:
It might make sense to drop JointStore since MongoDB has Views which
basically do the same thing: https://docs.mongodb.com/manual/core/views/
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#62?email_source=notifications&email_token=AAWWWRC3ASYVWPX7DKQ7PFTQ3YXT5A5CNFSM4JKOXYF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH7BGSQ#issuecomment-570299210>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWWWRCKDJL7BLDD3IH2Y2TQ3YXT5ANCNFSM4JKOXYFQ>
.
|
I think the View can be used as a normal Mongo Collection without write access. |
Oh, I think I understand better now — are Views persistent then? In other words, we create any views we want ahead of time and then connect to them as if they’re regular mongo collections?
… On Jan 2, 2020, at 11:05 AM, Shyam Dwaraknath ***@***.***> wrote:
I think the View can be used as a normal Mongo Collection without write access.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#62?email_source=notifications&email_token=AAWWWRDEMJ6DDV4DFSKV4NLQ3Y3HTA5CNFSM4JKOXYF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH7DOPY#issuecomment-570308415>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWWWRAZR2ZNSK3FCRJGRVDQ3Y3HTANCNFSM4JKOXYFQ>.
|
This is a structural refactoring and doesn't complete the process of getting to 1.0. There are additional goals in the documentation and some additional features necessary for that, but those will come in their own PRs.
The big goals are to:
replace MPI with zeroMQEdit:
Reducing coverage goal to 85% as 100% is not-realistic for now.
Scrapping plans for zeroMQ multi-node implementation as this will likely require some async magic