Move neural nets factory out of utils #994

famura · 2024-03-15T14:27:44Z

What does this implement/fix? Explain your changes

...

Does this close any currently open issues?

Fixes #743

Any relevant code examples, logs, error output, etc?

...

Any other comments?

...

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

I have read and understood the contribution
guidelines
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
I have reported how long the new tests run and potentially marked them
with pytest.mark.slow.
New and existing unit tests pass locally with my changes
I performed linting and formatting as described in the contribution
guidelines
I rebased on main (or there are no conflicts with main)

codecov · 2024-03-15T14:38:03Z

Codecov Report

Attention: Patch coverage is 75.60976% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 76.97%. Comparing base (4c2f71e) to head (1ee83f8).
Report is 15 commits behind head on main.

Files	Patch %	Lines
sbi/neural_nets/factory.py	83.05%	10 Missing ⚠️
sbi/utils/get_nn_models.py	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
+ Coverage   76.37%   76.97%   +0.59%     
==========================================
  Files          84       89       +5     
  Lines        6507     6575      +68     
==========================================
+ Hits         4970     5061      +91     
+ Misses       1537     1514      -23

Flag	Coverage Δ
unittests	`76.97% <75.60%> (+0.59%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

famura · 2024-03-18T16:41:39Z

Any idea why the coverage failed @janfb @michaeldeistler ?

Could it be cause this PR is from my fork?

manuelgloeckler · 2024-03-18T17:02:55Z

Hey @famura,
the coverage tests can fail because GitHub actions do not run all tests i.e. not GPU and not SLOW.
As you changed a lot of files this can lead to failure of this check.

It is fine for this to happen. Dont be confused by it :)

famura · 2024-03-18T17:39:49Z

Thanks for the explanation, much appreciated 🙂

…ral_nets_flow

famura · 2024-03-19T14:04:00Z

FYI, the information by codecov earlier is outdated. we actually increased the coverage now (a bit).

michaeldeistler

Awesome! Made minor comments regarding wording, but good to go afterwards.

Please run slow tests though:
pytest -m "not gpu" tests

michaeldeistler · 2024-03-19T14:02:41Z

docs/docs/reference.md

    rendering:
      show_root_heading: true
      show_object_full_path: true

-::: sbi.utils.get_nn_models.classifier_nn
+::: sbi.neural_nets.factory.classifier_nn


how about inference_factory? factory seems a bit too generic for me, seeing that embedding_nets are also built in neural_nets.

I am happy to change the name of the file to inference_factory. Here are the reasons why I went with the more generic name:

The factory.py lives in the neural_nets module, thus, (too me) it is clear that these factory functions are meant/tailored to all nets in this module. Other modules could theoretically have their own factory.py.

it's not user-face anyway since the user can not directly import from sbi.neural_nets.

Do you still want me to change it?

sbi/utils/get_nn_models.py

tests/neural_nets_factory.py

famura · 2024-03-19T14:49:05Z

Please run slow tests though: pytest -m "not gpu" tests

The tests are still running. Maybe we could think about distributed testing 😄

famura · 2024-03-19T15:24:23Z

@michaeldeistler so this is the output of all (including slow) tests without GPU. I believe the error to be independent of my changes

============================================================ XPASSES =============================================================
__________________________________________________ test_picklability[SNRE_B-vi] __________________________________________________
------------------------------------------------------ Captured stdout call ------------------------------------------------------
 Training neural network. Epochs trained: 2
==================================================== short test summary info =====================================================
SKIPPED [4] ../sbi/tests/mcmc_slice_pyro/test_slice.py:140: Slow test (https://github.com/pytorch/pytorch/issues/12190)
SKIPPED [2] ../sbi/tests/mcmc_slice_pyro/test_slice.py:265: Slice sampling not implemented for multiple sites yet.
SKIPPED [2] ../sbi/tests/mcmc_slice_pyro/test_slice.py:290: Slice sampling not implemented for multiple sites yet.
SKIPPED [2] ../sbi/tests/mcmc_slice_pyro/test_slice.py:324: Slice sampling not implemented for multiple sites yet.
SKIPPED [3] ../sbi/tests/mcmc_slice_pyro/test_slice.py:349: Slice sampling not implemented for multiple sites yet.
SKIPPED [2] ../sbi/tests/mcmc_slice_pyro/test_slice.py:413: Slice sampling not implemented for multiple sites yet.
SKIPPED [2] ../sbi/tests/mcmc_slice_pyro/test_slice.py:454: Slice sampling not implemented for multiple sites yet.
XFAIL tests/ensemble_test.py::test_c2st_posterior_ensemble_on_linearGaussian[SNPE_C-5]
XFAIL tests/inference_on_device_test.py::test_validate_theta_and_x_shapes[shape_theta1-shape_x0]
XFAIL tests/inference_with_NaN_simulator_test.py::test_inference_with_nan_simulator[SNLE_A-0.05]
XFAIL tests/inference_with_NaN_simulator_test.py::test_inference_with_nan_simulator[SNRE_B-0.05]
XFAIL tests/linearGaussian_snpe_test.py::test_c2st_multi_round_snpe_on_linearGaussian[snpe_b] - SNPE-B not implemented
XFAIL tests/linearGaussian_snpe_test.py::test_api_force_first_round_loss[False-False]
XFAIL tests/posterior_nn_test.py::test_log_prob_with_different_x[2-SNPE_A]
XFAIL tests/posterior_nn_test.py::test_log_prob_with_different_x[2-SNPE_C]
XFAIL tests/user_input_checks_test.py::test_process_prior[prior0]
XFAIL tests/user_input_checks_test.py::test_process_prior[prior1]
XFAIL tests/user_input_checks_test.py::test_process_prior[prior2]
XFAIL tests/user_input_checks_test.py::test_process_prior[prior3]
XFAIL tests/user_input_checks_test.py::test_process_x[x3-None-False]
XFAIL tests/user_input_checks_test.py::test_independent_joint_shapes_and_samples[dists0]
XFAIL tests/user_input_checks_test.py::test_independent_joint_shapes_and_samples[dists1]
XFAIL tests/user_input_checks_test.py::test_independent_joint_shapes_and_samples[dists2]
XFAIL tests/user_input_checks_test.py::test_independent_joint_shapes_and_samples[dists3]
XFAIL tests/user_input_checks_test.py::test_passing_custom_density_estimator[nn] - custom density estimator must be a function return nn.Module, not the nn.Module.
XPASS tests/save_and_load_test.py::test_picklability[SNRE_B-vi]
FAILED tests/torchutils_test.py::test_process_device[cuda] - AssertionError: assert 'cuda' == 'cuda:0'
1 failed, 814 passed, 17 skipped, 330 deselected, 18 xfailed, 1 xpassed, 859 warnings in 2225.48s (0:37:05)

janfb · 2024-03-19T16:13:09Z

Yes, the test_process_device failure is not related to your changes. It seems to be an error in the tests.

Can you please tell me the line where it fails (I cannot test it locally)?

famura · 2024-03-19T16:27:14Z

Sure, it is /sbi/tests/torchutils_test.py:219

...............xxxx......x.............................................................................................    [100%]
============================================================ FAILURES ============================================================
___________________________________________________ test_process_device[cuda] ____________________________________________________

device_input = 'cuda'

    @pytest.mark.parametrize("device_input", ("cpu", "gpu", "cuda", "cuda:0", "mps"))
    def test_process_device(device_input: str) -> None:
        """Test whether the device is processed correctly."""
    
        try:
            device_output = torchutils.process_device(device_input)
            if device_input == "cpu":
                assert device_output == "cpu"
            elif device_input == "gpu":
                if torch.cuda.is_available():
                    current_gpu_index = torch.cuda.current_device()
                    assert device_output == f"cuda:{current_gpu_index}"
                elif torch.backends.mps.is_available():
                    assert device_output == "mps:0"
    
            if device_input == "cuda" and torch.cuda.is_available():
>               assert device_output == "cuda:0"
E               AssertionError: assert 'cuda' == 'cuda:0'
E                 
E                 - cuda:0
E                 ?     --
E                 + cuda

../sbi/tests/torchutils_test.py:219: AssertionError

jnsbck · 2024-03-19T18:13:48Z

It'd be awesome if we could get this merged rather soon, since #989 depends on this, which I would love to work on. :)

janfb

great effort @famura 👏
Added a comment regarding formatting changes. Smells like different ruff or pre-commit versions 😬
I am indifferent regarding the factory naming @michaeldeistler

tests/inference_on_device_test.py

janfb · 2024-03-20T06:04:21Z

tests/neural_nets_factory.py

+
+
+@pytest.mark.parametrize(
+    "model", ["linear", "mlp", "resnet"], ids=["linear", "mlp", "resnet"]


I have not seen the ids arg before, what is it doing?

So ids gives human-readable names that can be used to further subselect tests. For example pytest tests/neural_nets_factory.py::test_deprecated_import_classifier_nn runs all 3, but pytest tests/neural_nets_factory.py::test_deprecated_import_classifier_nn -k "linear" only the one with the linear model. The real power arises when you give ids to every parameter and want to check on the scenario.
I enjoy the integration with my IDE since it makes it faster to see which test combinations crash.

janfb · 2024-03-20T06:05:46Z

sbi/utils/get_nn_models.py

-        num_components: Number of mixture components for a mixture of Gaussians.
-            Ignored if density estimator is not an mdn.
-        kwargs: additional custom arguments passed to downstream build functions.
+    r"""This method is deprecated and will be removed in the next release.


Not sure we will remove it so noon. Let's say "in a future release"

janfb · 2024-03-20T06:09:14Z

Sure, it is /sbi/tests/torchutils_test.py:219

...............xxxx......x.............................................................................................    [100%]
============================================================ FAILURES ============================================================
___________________________________________________ test_process_device[cuda] ____________________________________________________

device_input = 'cuda'

    @pytest.mark.parametrize("device_input", ("cpu", "gpu", "cuda", "cuda:0", "mps"))
    def test_process_device(device_input: str) -> None:
        """Test whether the device is processed correctly."""
    
        try:
            device_output = torchutils.process_device(device_input)
            if device_input == "cpu":
                assert device_output == "cpu"
            elif device_input == "gpu":
                if torch.cuda.is_available():
                    current_gpu_index = torch.cuda.current_device()
                    assert device_output == f"cuda:{current_gpu_index}"
                elif torch.backends.mps.is_available():
                    assert device_output == "mps:0"
    
            if device_input == "cuda" and torch.cuda.is_available():
>               assert device_output == "cuda:0"
E               AssertionError: assert 'cuda' == 'cuda:0'
E                 
E                 - cuda:0
E                 ?     --
E                 + cuda

../sbi/tests/torchutils_test.py:219: AssertionError

Moved to #1047 as this is independent from the changes in this PR.

famura · 2024-03-20T08:48:44Z

So is this PR ready to be merged in your opinion?

jnsbck · 2024-03-20T10:27:28Z

awesome! thanks for the effort! 🚀

famura added 4 commits March 18, 2024 16:15

Export classes and function

5c5acdf

Reworked factory imports

efef924

Debugging ruff

7f226b9

Debugging ruff

9160cbf

famura marked this pull request as ready for review March 18, 2024 16:06

famura added hackathon architecture Internal changes without API consequences labels Mar 18, 2024

famura self-assigned this Mar 18, 2024

famura added the bug Something isn't working label Mar 18, 2024

famura marked this pull request as draft March 19, 2024 08:03

famura added 2 commits March 19, 2024 10:04

Added links to the new location with deprecation warnings

9483913

Forgot build_zuko_maf

8ddac04

famura mentioned this pull request Mar 19, 2024

Running import sbi.neural_nets.mdn on main raises ImportError #1011

Closed

Added tests for the deprecated functions

7239730

famura marked this pull request as ready for review March 19, 2024 10:59

Merge remote-tracking branch 'upstream/main' into fix/circ_import_neu…

3544d7a

…ral_nets_flow

michaeldeistler approved these changes Mar 19, 2024

View reviewed changes

famura added 3 commits March 19, 2024 15:38

Renaming

75f85a0

Doc strings now only contain the deprecation warning

45ddfd3

Updated the deprecation message

c4f3cc2

janfb reviewed Mar 20, 2024

View reviewed changes

Updated the deprecation message

afc685d

Ruff once more

1ee83f8

janfb merged commit bae6994 into sbi-dev:main Mar 20, 2024
4 of 5 checks passed

famura deleted the fix/circ_import_neural_nets_flow branch March 20, 2024 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move neural nets factory out of utils #994

Move neural nets factory out of utils #994

famura commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

famura commented Mar 18, 2024 •

edited

Loading

manuelgloeckler commented Mar 18, 2024

famura commented Mar 18, 2024

famura commented Mar 19, 2024

michaeldeistler left a comment

michaeldeistler Mar 19, 2024

famura Mar 19, 2024

famura commented Mar 19, 2024 •

edited

Loading

famura commented Mar 19, 2024

janfb commented Mar 19, 2024

famura commented Mar 19, 2024

jnsbck commented Mar 19, 2024 •

edited

Loading

janfb left a comment

janfb Mar 20, 2024

famura Mar 20, 2024

janfb Mar 20, 2024

janfb commented Mar 20, 2024

famura commented Mar 20, 2024

jnsbck commented Mar 20, 2024



		@pytest.mark.parametrize(
		"model", ["linear", "mlp", "resnet"], ids=["linear", "mlp", "resnet"]

Move neural nets factory out of utils #994

Move neural nets factory out of utils #994

Conversation

famura commented Mar 15, 2024 • edited Loading

What does this implement/fix? Explain your changes

Does this close any currently open issues?

Any relevant code examples, logs, error output, etc?

Any other comments?

Checklist

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

famura commented Mar 18, 2024 • edited Loading

manuelgloeckler commented Mar 18, 2024

famura commented Mar 18, 2024

famura commented Mar 19, 2024

michaeldeistler left a comment

Choose a reason for hiding this comment

michaeldeistler Mar 19, 2024

Choose a reason for hiding this comment

famura Mar 19, 2024

Choose a reason for hiding this comment

famura commented Mar 19, 2024 • edited Loading

famura commented Mar 19, 2024

janfb commented Mar 19, 2024

famura commented Mar 19, 2024

jnsbck commented Mar 19, 2024 • edited Loading

janfb left a comment

Choose a reason for hiding this comment

janfb Mar 20, 2024

Choose a reason for hiding this comment

famura Mar 20, 2024

Choose a reason for hiding this comment

janfb Mar 20, 2024

Choose a reason for hiding this comment

janfb commented Mar 20, 2024

famura commented Mar 20, 2024

jnsbck commented Mar 20, 2024

famura commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

famura commented Mar 18, 2024 •

edited

Loading

famura commented Mar 19, 2024 •

edited

Loading

jnsbck commented Mar 19, 2024 •

edited

Loading