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

Update tensorflow-probability #56

Merged
merged 21 commits into from
Feb 20, 2025
Merged

Update tensorflow-probability #56

merged 21 commits into from
Feb 20, 2025

Conversation

ehfd
Copy link
Member

@ehfd ehfd commented Feb 5, 2025

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Closes #53
Closes #54
Closes #55

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 5, 2025

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • requirements: run_constrained: tensorflow-datasets >= 2.2.0 should not contain a space between relational operator and the version, i.e. tensorflow-datasets >=2.2.0

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159011292. Examine the logs at this URL for more detail.

conda-forge-webservices[bot] and others added 2 commits February 5, 2025 13:59
@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 5, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ Detected pure Python wheel(s) in source: https://pypi.org/packages/py2.py3/t/tensorflow-probability/tensorflow_probability-{{ version }}-py2.py3-none-any.whl. This is generally ok for pure Python wheels and noarch=python packages but it's preferred to use a source distribution (sdist) if possible.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159706390. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159558716. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159720112. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • ❌ The test section contained an unexpected subsection name. command is not a valid subsection name.

For recipe/meta.yaml:

  • ℹ️ Detected pure Python wheel(s) in source: https://pypi.org/packages/py2.py3/t/tensorflow-probability/tensorflow_probability-{{ version }}-py2.py3-none-any.whl. This is generally ok for pure Python wheels and noarch=python packages but it's preferred to use a source distribution (sdist) if possible.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159874562. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159879563. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 5, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.
    • For the test.requires section of recipe, you should usually use python {{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).
  • ℹ️ Detected pure Python wheel(s) in source: https://pypi.org/packages/py2.py3/t/tensorflow-probability/tensorflow_probability-{{ version }}-py2.py3-none-any.whl. This is generally ok for pure Python wheels and noarch=python packages but it's preferred to use a source distribution (sdist) if possible.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13436159965. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13159962243. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 5, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13160118693. Examine the logs at this URL for more detail.

@ehfd ehfd marked this pull request as ready for review February 5, 2025 16:04
@ehfd
Copy link
Member Author

ehfd commented Feb 16, 2025

@jonas-eschle Need to test the previous tensorflow-probability version on 2.18, so let's wait until tf-keras 2.18 is added.

@ehfd
Copy link
Member Author

ehfd commented Feb 16, 2025

The error message when you use Tensorflow 2.17.0 with 0.25.0 for people in the future (0.24.0 works fine):

ImportError: This version of TensorFlow Probability requires TensorFlow version >= 2.18; Detected an installation of version 2.17.0. Please upgrade TensorFlow to proceed.

@ehfd
Copy link
Member Author

ehfd commented Feb 16, 2025

@jonas-eschle

So conclusion (I've built the versions and tried it for real in conda):

This version depends on tf-keras >=2.18 so this can only be merged when that is merged.
TensorFlow has explicit import errors when the version is lower than designated (2.18.0 for 0.25.0).
JAX version requirements should NOT be lowered, as it breaks at any time.

For whether the upward bound dependency constraints can be relieved, need to test tensorflow-probability 0.24.0 with TensorFlow 2.18.0, and test JAX 0.5.x on tensorflow-probability 0.24.0 and 0.25.0. Both need to wait for respective feedstocks.

@jonas-eschle
Copy link
Contributor

@jonas-eschle

So conclusion: This version depends on tf-keras >=2.18 so this can only be merged when that is merged. TensorFlow has explicit import errors when the version is lower than designated (2.18.0 for 0.25.0). JAX version requirements should NOT be lowered, as it breaks at any time.

Agree. We just don't know about JAX, but I don't think it's a disadvantage, really. If you wat to upgrade, why not also upgrade JAX? And if there is a good reason not to, i.e. because JAX had breaking changes, chances are, that exactly these will also break TFP. So yes, agree.

For whether the upward bound dependency constraints can be relieved, need to test tensorflow-probability 0.24.0 with TensorFlow 2.18.0, and test JAX 0.5.x on tensorflow-probability 0.24.0 and 0.25.0. Both need to wait for respective feedstocks.

Agree. I would suggest not to make these "blocking" items for now: as soon as TF-Keras is out, we should release this. Preferrably with the restrictions in place (the jump to 0.5 could indicate something larger, so maybe we make one at a time/resubmit an increased boundary/no boundary if we're confident. Always ugly if code breaks at runtime error and you need to figure out that this is due to a version mismatch, i.e. too new JAX version for example). Much harder than finding a way around the dependency.

@ehfd
Copy link
Member Author

ehfd commented Feb 18, 2025

Agree. We just don't know about JAX, but I don't think it's a disadvantage, really. If you wat to upgrade, why not also upgrade JAX? And if there is a good reason not to, i.e. because JAX had breaking changes, chances are, that exactly these will also break TFP. So yes, agree.

"conda-forge JAX maintainer" sounds nice, I'll try.

@ehfd
Copy link
Member Author

ehfd commented Feb 18, 2025

And yeah, we should merge immediately when tf-keras is merged.

@ehfd
Copy link
Member Author

ehfd commented Feb 18, 2025

@jonas-eschle

"conda-forge JAX maintainer" sounds nice, I'll try.

It seems that something crazy is going on to make jaxlib (https://github.com/conda-forge/jaxlib-feedstock/pull/294/files) work and I don't think I'm really ready for this yet.

I still don't understand Bazel well enough and the peripheral TensorFlow packages (mostly tensorflow-datasets, array-record, tensorflow-text, etc.) were supposed to be an opportunity to get deeper into this before being introduced into the more important packages.

@jonas-eschle
Copy link
Contributor

It seems that something crazy is going on to make jaxlib (conda-forge/jaxlib-feedstock#294 (files)) work and I don't think I'm really ready for this yet.

I still don't understand Bazel well enough and the peripheral TensorFlow packages (mostly tensorflow-datasets, array-record, tensorflow-text, etc.) were supposed to be an opportunity to get deeper into this before being introduced into the more important packages.

Same here, my knowledge of Bazel is very limited. But it seems as if it works, so let's see! (btw, if you wanna just test, you can use the pip version, right?)

@ehfd
Copy link
Member Author

ehfd commented Feb 19, 2025

if you wanna just test, you can use the pip version, right?

You are right, I just made the same assumptions as TensorFlow where you need to use a Conda build to verify CUDA, when the topic here isn't CUDA.

@ehfd
Copy link
Member Author

ehfd commented Feb 19, 2025

@jonas-eschle Apparently, the case is that JAX 0.5.0 (pip) with TensorFlow Probability 0.25.0 (pip) does not introduce substantial backwards incompatible changes in its interface.

test_jax.tar.gz

E external/local_xla/xla/stream_executor/cuda/cuda_fft.cc:477] Unable to register cuFFT factory: Attempting to register factory for plugin cuFFT when one has already been registered
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
E0000 00:00:1739995898.518939 3937547 cuda_dnn.cc:8310] Unable to register cuDNN factory: Attempting to register factory for plugin cuDNN when one has already been registered
E0000 00:00:1739995898.526076 3937547 cuda_blas.cc:1418] Unable to register cuBLAS factory: Attempting to register factory for plugin cuBLAS when one has already been registered
JAX version: 0.5.0
TensorFlow Probability version: 0.25.0

--- Testing Distribution Functionality ---
Normal samples: [ 1.6226422   2.0252647  -0.43359444 -0.07861735  0.1760909 ]
Log probabilities: [-2.2354221  -2.9697871  -1.0129406  -0.92202884 -0.9344425 ]
Mean: 0.0 Variance: 1.0

--- Testing JIT Compilation ---
JIT-compiled function log probabilities: [-1.4189385 -1.0439385 -0.9189385 -1.0439385 -1.4189385]

--- Testing Automatic Differentiation ---
Gradient of loss with respect to scale (at scale=1.0): 0.047701687
E external/local_xla/xla/stream_executor/cuda/cuda_fft.cc:477] Unable to register cuFFT factory: Attempting to register factory for plugin cuFFT when one has already been registered
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
E0000 00:00:1739996359.091980 3941103 cuda_dnn.cc:8310] Unable to register cuDNN factory: Attempting to register factory for plugin cuDNN when one has already been registered
E0000 00:00:1739996359.103729 3941103 cuda_blas.cc:1418] Unable to register cuBLAS factory: Attempting to register factory for plugin cuBLAS when one has already been registered
......
----------------------------------------------------------------------
Ran 6 tests in 4.252s

OK

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

@jonas-eschle The combination of TensorFlow 2.18.0 (conda-forge) and tf-keras 2.18.0 (pip) with TensorFlow Probability 0.24.0 (the older version) did not present any issues.

test_tf.tar.gz

E external/local_xla/xla/stream_executor/cuda/cuda_fft.cc:477] Unable to register cuFFT factory: Attempting to register factory for plugin cuFFT when one has already been registered
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
E0000 00:00:1740050377.882254   34256 cuda_dnn.cc:8310] Unable to register cuDNN factory: Attempting to register factory for plugin cuDNN when one has already been registered
E0000 00:00:1740050377.889672   34256 cuda_blas.cc:1418] Unable to register cuBLAS factory: Attempting to register factory for plugin cuBLAS when one has already been registered
I tensorflow/core/platform/cpu_feature_guard.cc:210] This TensorFlow binary is optimized to use available CPU instructions in performance-critical operations.
To enable the following instructions: SSE4.1 SSE4.2 AVX AVX2 AVX512F FMA, in other operations, rebuild TensorFlow with the appropriate compiler flags.
TensorFlow version: 2.18.0
TensorFlow Probability version: 0.24.0
I0000 00:00:1740050381.406957   34256 gpu_device.cc:2022] Created device /job:localhost/replica:0/task:0/device:GPU:0 with 15212 MB memory:  -> device: 0, name: Quadro RTX 6000, pci bus id: 0000:3b:00.0, compute capability: 7.5

[Basic Distribution]
Sample mean = 0.011089784
Average log probability = -1.4298217

[Joint Distribution]
Joint sample (x, y): [np.float32(0.58516246), np.float32(1.5285759)]

[Bayesian Linear Regression via HMC]
Posterior mean estimate of a: 1.207
Posterior mean estimate of b: 0.742
Posterior mean estimate of sigma: 0.011
Matplotlib not installed; skipping plots.
E external/local_xla/xla/stream_executor/cuda/cuda_fft.cc:477] Unable to register cuFFT factory: Attempting to register factory for plugin cuFFT when one has already been registered
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
E0000 00:00:1740050411.463516   34979 cuda_dnn.cc:8310] Unable to register cuDNN factory: Attempting to register factory for plugin cuDNN when one has already been registered
E0000 00:00:1740050411.471401   34979 cuda_blas.cc:1418] Unable to register cuBLAS factory: Attempting to register factory for plugin cuBLAS when one has already been registered
I tensorflow/core/platform/cpu_feature_guard.cc:210] This TensorFlow binary is optimized to use available CPU instructions in performance-critical operations.
To enable the following instructions: SSE4.1 SSE4.2 AVX AVX2 AVX512F FMA, in other operations, rebuild TensorFlow with the appropriate compiler flags.
I0000 00:00:1740050414.886086   34979 gpu_device.cc:2022] Created device /job:localhost/replica:0/task:0/device:GPU:0 with 15212 MB memory:  -> device: 0, name: Quadro RTX 6000, pci bus id: 0000:3b:00.0, compute capability: 7.5
Normal Distribution Mean: 0.0
Normal Distribution Variance: 1.0
Sample from Normal: [ 0.73769367  1.1850613  -2.313611    0.2876023   0.51588637]
Log Probabilities: [-0.9189385 -1.4189385 -1.4189385]
CDF Values: [0.5        0.8413447  0.15865527]
Lognormal Sample: [2.7763894  0.54644555 0.6659182  2.9804587  2.1727817 ]
Joint Distribution Sample: [<tf.Tensor: shape=(), dtype=float32, numpy=1.496046543121338>, <tf.Tensor: shape=(), dtype=float32, numpy=0.6264479160308838>]
Log Probability of Joint Sample: -2.7930865
Estimated Mean: -0.042495437
Estimated Variance: 0.9843308
TensorFlow Probability tests completed successfully.

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

@conda-forge-admin, please rerender

@jonas-eschle
Copy link
Contributor

jonas-eschle commented Feb 20, 2025

@jonas-eschle The combination of TensorFlow 2.18.0 (conda-forge) and tf-keras 2.18.0 (pip) with TensorFlow Probability 0.24.0 (the older version) did not present any issues.

Yes, however, this is just testing a tiny subset of the functionality. If we want to assure that things work, we would need to run at least the whole test suite. And ideally on different machines, i.e. Linux, OSX, Windows....
That's by far not enough to know by now.

Also, just because the old one did not break does not mean that newer version won't break.

So I would still say for JAX, if they do a major upgrade to 0.5, it's save to not yet release it. For TF, I am less sure what (and if) to constrain it.
(For JAX, we would need to run at least the whole test suite)

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13434217828. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

So I would still say for JAX, if they do a major upgrade to 0.5, it's save to not yet release it.

I will reverting the constraints for JAX.
However, the issue here is the current constraints restrict available packages for TensorFlow 2.17.x and potentially 2.19.x in the future. It is suspected that pip users are fine with loosened constraints.

@jonas-eschle
Copy link
Contributor

However, the issue here is the current constraints restrict available packages for TensorFlow 2.17.x

True. But this isn't our choice, right? TFP simply won't work wit a lower version, it's in the code

It is suspected that pip users are fine with loosened constraints.

Actually, good argument. Pip has the same problem. Why be more constraint here. I am fine with removing the upper constraint for JAX as well, the argument convinced me now

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

However, the issue here is the current constraints restrict available packages for TensorFlow 2.17.x
True. But this isn't our choice, right? TFP simply won't work wit a lower version, it's in the code

The argument is not with the past, but the future (combination of TensorFlow 2.19.x potentially with TensorFlow Probability 0.25.0 when they decide to skip a matched release again).

Actually, good argument. Pip has the same problem. Why be more constraint here. I am fine with removing the upper constraint for JAX as well, the argument convinced me now

Sure, I will remove the upper bound constraints again.

An older TensorFlow version then 2.18.0 is blocked, obviously, and key capabilities in JAX breaks with TensorFlow Probability 0.25.0 at around version 0.4.24 or 0.4.23 (I have tested so). So decreasing the lower limits is risky, and should be kept, whether or not pip enforces it. But the upper limits - I doubt there will be issues.

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13435906324. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13436174385. Examine the logs at this URL for more detail.

@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

@jonas-eschle And the builds succeed...

I think if there is an issue with the upper bounds somehow, we can limit it when there is an issue, since pip would be under the same problem (which technically becomes the project's bug).

@jonas-eschle jonas-eschle merged commit 02b94f9 into conda-forge:main Feb 20, 2025
5 checks passed
@jonas-eschle
Copy link
Contributor

An older TensorFlow version then 2.18.0 is blocked, obviously, and key capabilities in JAX breaks with TensorFlow Probability 0.25.0 at around version 0.4.24 or 0.4.23 (I have tested so). So decreasing the lower limits is risky, and should be kept, whether or not pip enforces it. But the upper limits - I doubt there will be issues.

Very well, I may misunderstood, but we agree.

I think if there is an issue with the upper bounds somehow, we can limit it when there is an issue, since pip would under the same problem.

It's not that easy, and the argument also works the other way around: you can relax it once we know it works. I am saying that it's not that easy, because how do you know it broke because of a mismatched version? A user may randomly bumps into it. But that's not a version error. That's a random error. And the user tracks it down, may first thinks it's him. If he is a power user, he can track it down and, fair enough, report it maybe in TFP (or here). But there are a lot of ifs. Most notably, if will cause hard to understand bugs for users.

But we agree, LGTM! Many thanks for this contribution!

@ehfd ehfd deleted the pr branch February 20, 2025 13:43
@ehfd
Copy link
Member Author

ehfd commented Feb 20, 2025

Yeah, learned a lot of things in the progress. Thanks for your continued patience. @jonas-eschle

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.

Excessive dependencies
3 participants