Remove arviz as a dependency

zaxtax commented 1 year ago

Motivation

This removes arviz as an explicit dependency of Bean Machine. This will also prevent a circular dependency with arviz.

Changes proposed

Updates setup.py and moves import into to_inference_data

Types of changes

[ ] Docs change / refactoring / dependency upgrade
[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.
[x] The title of my pull request is a short description of the requested changes.

zaxtax commented 1 year ago

I'm happy to leave in the arviz import. Also with the contents of to_inference_data merged, we could remove it as well.

On Thu, 15 Dec 2022, 20:49 Xiaoyan Wang, @.***> wrote:

@.**** requested changes on this pull request.

Removing ArviZ dependency sounds fine to me, though in this case we should also remove the global import in monte_carlo_sample.py https://github.com/facebookresearch/beanmachine/blob/a411c6efea2955380504752349bffcfb73a0e3a6/src/beanmachine/ppl/inference/monte_carlo_samples.py#L8 (because this module will be imported when people import beanmachine, and we don't want it to raise an ModuleNotFoundError due to missing ArviZ dependency.

We can consider moving the ArviZ import into to_inference_data method for now, so that the ArviZ dependency is only required if users need to do the conversion.

This will also prevent a circular dependency with arviz.

Looks like ArviZ only has Bean Machine as an external dependency, so even if we keep ArviZ as-is, this shouldn't cause any circular dependency issue. In fact, it looks like PyMC also depends on ArviZ https://github.com/pymc-devs/pymc/blob/main/requirements.txt#L1 at the moment and that works just fine for them

— Reply to this email directly, view it on GitHub https://github.com/facebookresearch/beanmachine/pull/1822#pullrequestreview-1219853187, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCUNT7HWIKX73WAKGVVTWNNY5JANCNFSM6AAAAAAR5C35RA . You are receiving this because you authored the thread.Message ID: @.***>

ndmlny-qs commented 1 year ago

We could also use the _requires_dev_packages in

https://github.com/facebookresearch/beanmachine/blob/c80d1a4d48e1fb124fd93d0b15c0fdbac7037c9c/src/beanmachine/ppl/diagnostics/tools/viz.py#L20

as a decorator for the to_inference_data method found in

https://github.com/facebookresearch/beanmachine/blob/a411c6efea2955380504752349bffcfb73a0e3a6/src/beanmachine/ppl/inference/monte_carlo_samples.py#L269

Using this, I think we can remove the ArviZ import, and if the user wanted to call the .to_inference_data method, then they would get the very useful error message indicating that they need to also install the dev requirements for Bean Machine.

facebookresearch / beanmachine