Skip to content

Comments

python3Packages.tensorflow-probability: 0.8.0 -> 0.15.0#156027

Merged
SuperSandro2000 merged 7 commits intoNixOS:masterfrom
simeoncarstens:fix-tfp
Jan 29, 2022
Merged

python3Packages.tensorflow-probability: 0.8.0 -> 0.15.0#156027
SuperSandro2000 merged 7 commits intoNixOS:masterfrom
simeoncarstens:fix-tfp

Conversation

@simeoncarstens
Copy link
Contributor

@simeoncarstens simeoncarstens commented Jan 21, 2022

Motivation for this change

python3Packages.tensorflow-probability was outdated and marked as broken because of an unpackaged dependency (python3Packages.dm-tree). dm-tree is now packaged (#152971), so I updated tensorflow-probability to the most current version.
Several python3Packages depend on tensorflow-probability, namely dm-sonnet, arviz, and pymc3 and fixing tensorflow-probability thus potentially "unbreaks" them. Turns out that

  • arviz tests require an older version of tensorflow-probability,
  • pymc3 now is broken for a different reason (missing dependencies),
  • dm-sonnet is broken for a different reason (requires an older tensorflow version than the one currently packaged).

So I disabled the arviz tests requiring tensorflow-probability and marked pymc3 and dm-sonnet as broken.
Furthermore, tensorflow-tensorboard is a dependency of tensorflow-probability, but is currently incompatible with Python 3.10. I thus disabled tensorflow-tensorboard for Python 3.10.
I also took the liberty to add the numba-related tests to the list of tests run for arviz.

Finally, unfortunately tensorflow-probability is maintainerless. I would love to get engaged and maintain it, but I feel I don't have the necessary Nix skills quite yet to do / review more difficult changes , especially with regards to the Bazel tooling or the hacking likely required to run the unit tests.

This is my first PR here, so please let me know in case I am breaking any protocols or how to improve / do better next time.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 21, 2022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required? Broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added / cleaned up some of the comments. Hope it's all clearer now.

@ofborg ofborg bot added the 8.has: clean-up This PR removes packages or removes other cruft label Jan 24, 2022
@ofborg ofborg bot requested review from OmnipotentEntity and timokau January 24, 2022 09:43
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 24, 2022
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 156027 run on x86_64-linux 1

3 packages marked as broken and skipped:
  • python39Packages.dm-sonnet
  • python39Packages.graph_nets
  • python39Packages.pymc3
1 package failed to build and are new build failure:

tensorflow-probability 0.8 was broken and marked as such (in NixOS#108977)
because of a dependency on dm-tree, which seems not to have been
available.
Since dm-tree is now available in nixpkgs (NixOS#152971),
tensorflow-probability is easy to fix and to upgrade.
tensorboard is currently not compatible with Python 3.10, but will be
in the next release (tensorflow/tensorboard#5478).
The tests in this version of arviz require an older version of
TensorFlow Probability than the one packaged in this PR.
This also fixes a few missing dependencies.
This version of dm-sonnet depends on an older TensorFlow version than
currently packaged.
Several dependencies are missing and finally, this version requires
the theano-pymc3 Python package, which is currently not packaged.
@simeoncarstens
Copy link
Contributor Author

Since the merging of #154708, building arviz fails, via #152660. Looks like I should have used NixOS:staging as the base branch, right?
Anyways, arviz 0.11.4 pins typing_extensions>=3.7.4.3,<4. In the next release, this is not the case anymore (arviz-devs/arviz#1948). I'm not sure what the protocol is - a simple

patchPhase = ''
  sed -i "s#typing_extensions>=3.7.4.3,<4#typing_extensions>=3.7.4.3#" requirements.txt
'';

makes it build (and pass the activated tests) perfectly fine, but it's probably out of scope for this PR, so I'll mark arviz as broken.

@SuperSandro2000
Copy link
Member

Looks like I should have used NixOS:staging as the base branch, right?

No, this PR rebuilds under 10 packages which means it goes to master.

makes it build (and pass the activated tests) perfectly fine, but it's probably out of scope for this PR, so I'll mark arviz as broken.

If the fix is as simple as that and you only need to commit it, it is a waste of time to mark it broken now and unmark it later and then fix it. We can also throw that into this PR.

Also you want to to use postPatch to not break patches and substituteInPlace because it logs if there is no match.

postPhase = ''
  substituteInPlace requirements.txt \
    --replace- "typing_extensions>=3.7.4.3,<4" "typing_extensions>=3.7.4.3"
'';

arviz v0.11.4 has, according to its requirements.txt, a dependency on
typing-extensions < 4. Currently, only typing-extensions v.4.0.1 are
available in nixpkgs, while the above version constraint seems
unnecessary, see this arviz PR: https://github.com/arviz-devs/arviz/pull/1948/files.
This also activates tests that use zarr and to which changes are made
in above PR. I thought those changes might be related to
unconstraining the typing-extensions version, but looks like they are not.
@simeoncarstens
Copy link
Contributor Author

Thanks for the helpful comments! I am now patching arviz instead of marking it as broken.

@SuperSandro2000 SuperSandro2000 merged commit cfeee25 into NixOS:master Jan 29, 2022
@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants