Add goal inference and time series projects #1831
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
facebook-github-bot
added
the
CLA Signed
|
Sweet! This looks really cool |
There was a problem hiding this comment.
- copyright notices will need to be modified, definitely a blocker. If you don't have time to change them, then assign an issue for me or rob to make the changes.
- there a several new type errors that
pyrecatches, but that is not a blocker - many imports are wrong e.g.
from beanmachine.facebook..., which I am assuming are due to the internal version being used.pyrewas also catching these - several style issues exist as well, but not a blocker
- i did not run the notebooks
- tests do not follow the top-level test folder structure (but not a blocker)
I couldn't read everything, but I suspect these features will remain experimental for a while. The only thing that would need to be changed is the copyright before approval
| Sequential Monte Carlo for Goal Inference | ||
| ################## | ||
|
|
||
| **Author: James Damewood** |
There was a problem hiding this comment.
There is no info here so it could be removed.
| @@ -0,0 +1,17 @@ | |||
| # (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. | |||
|
|
|||
| load("@fbcode_macros//build_defs:sphinx_wiki.bzl", "sphinx_wiki") | |||
There was a problem hiding this comment.
I'm assuming this is referring to an interal library. I don't see a reason why it should be here, but I also don't see a reason why it can't be left.
|
Thanks for the review @ndmlny-qs I'll fix them this week. Good catch on the imports, I now notice pyre is not being run as part of my old linter since I switched devservers. |
There was a problem hiding this comment.
Is there any plan to continue maintaining these modules in the future? If not, then I would prefer that we do not introduce additional artifacts to Bean Machine repo, otherwise it will be even harder to keep our build green. We spent quite a bit of time last year cleaning out unused experimental and legacy packages, so it'd be really great if we don't revert it back without a good reason. :)
In addition, even though both projects are under beanmachine directory internally, they are both standalone libraries instead of components of Bean Machine (I don't even think they interact with Bean Machine at all). If we are really going to publish them, it would make more sense to have them in their own repo.
|
I guess this is really a question around legal; since BM is strictly OSS and these projects were previously in BM but not pushed to github, it is more straightforward to make them a part of the project (since BM is already approved for that), and then spun off to their own libraries rather than going through the internal process to oss a new project under Meta's organization which IIRC took months with coordination across multiple pocs. WDYT? |
Motivation
Since there are fewer BM maintainers within Meta, I wanted to open source intern infra projects (by James and Zhen) built on BM for external research and future use. These are all implementations of publicly available papers and contain no internal data. I would like to get this in if tests pass and defer design and functionality questions to a future PR since these things are a bit time sensitive.
Test Plan
I've run this internally, but I'm having issues installing BM on my laptop now (I'll open a separate issue about that) so I'm going to defer to CI's testing. Note that the individual tests for the two experimental projects are not being run as part of our CI suite but since they are experimental, that is not a blocker atm.