conda-forge / keras-tuner-feedstock

A conda-smithy repository for keras-tuner.
BSD 3-Clause "New" or "Revised" License
1 stars 9 forks source link

Incorrect dependencies #35

Open DManowitz opened 2 months ago

DManowitz commented 2 months ago

Solution to issue cannot be found in the documentation.

Issue

According to the upstream repo, the only required dependencies for v1.4.7 are keras, packaging, requests, and kt-legacy. Why does this package have so many other dependencies?

Installed packages

N/A

Environment info

N/A
Anselmoo commented 2 months ago

According to the upstream repo, the only required dependencies for v1.4.7 are keras, packaging, requests, and kt-legacy. Why does this package have so many other dependencies?

That keras-tuner only relies on these four packages might not be entirely correct because of:

  1. There is the extra requirement for tensorflow as backend
  2. The requirements for scipy and scikit-learn for the Bayesian-Optimization

So, it might be tested if keras-tuner-feedstock will be broken if dependencies are removed.

DManowitz commented 2 months ago

That keras-tuner only relies on these four packages might not be entirely correct because of:

1. There is the extra requirement for [`tensorflow`](https://github.com/keras-team/keras-tuner/blob/8aa8dc2971d2858823dd21c5492f2f1478654eb0/setup.py#L60C5-L66) as backend

2. The requirements for [`scipy` and `scikit-learn`](https://github.com/keras-team/keras-tuner/blob/8aa8dc2971d2858823dd21c5492f2f1478654eb0/setup.py#L60-L90) for the [Bayesian-Optimization](https://github.com/keras-team/keras-tuner/blob/8aa8dc2971d2858823dd21c5492f2f1478654eb0/keras_tuner/tuners/bayesian.py#L17-L28)

So, it might be tested if keras-tuner-feedstock will be broken if dependencies are removed.

  1. keras should be specifying any backend it needs as a keras dependency, so this package should not be specifying any transitive dependencies, as those may change as the lower-level packages change. For example, keras v3 no longer needs Tensorflow as a backend; it can work with PyTorch or JAX.

  2. The setup.py file in the upstream repo marks bayesian as an extra install, not part of the base requirements. Thus, this feedstock should not include those dependencies as dependencies of this conda package.

Anselmoo commented 2 months ago

Then it might make sense to propose a PR for meta.yaml for incorporating points 1 and 2 via implicit-metapackages like https://github.com/conda-forge/tensorflow-feedstock/blob/main/recipe/meta.yaml#L140-L146

If the user wants to use sklearn_tuner.py or bayesian.py, this has to be captured in the formula without overcomplicating the usage?

DManowitz commented 2 months ago

Then it might make sense to propose a PR for meta.yaml for incorporating points 1 and 2 via implicit-metapackages like https://github.com/conda-forge/tensorflow-feedstock/blob/main/recipe/meta.yaml#L140-L146

If the user wants to use sklearn_tuner.py or bayesian.py, this has to be captured in the formula without overcomplicating the usage?

I will propose a PR shortly. However, to address your second point, I don't think that scipy or scikit-learn should be included in the recipe at all, since they are marked as extra dependencies in the upstream setup.py. There are many conda packages which only install required dependencies of their upstream package, not extra dependencies. The tensorflow recipe is a very complicated one which leads to multiple packages being created, with different dependencies for the packages. I think this package should just install the required dependencies and leave it to the user to install scipy and/or scikit-learn themselves if they want to use the additional functionality.

DManowitz commented 2 months ago

BTW, @Anselmoo, if you really want to insist on a version that includes scipy and scikit-learn as dependencies, perhaps this feedstock can use the approach of one like gymnasium and have 2 separate outputs: keras-tuner and keras-tuner-all, with only the keras-tuner-all conda package including the scipy and scikit-learn dependencies.

Anselmoo commented 2 months ago

Or https://github.com/conda-forge/black-feedstock/blob/main/recipe/meta.yaml via black-jupyter, @DManowitz I would consider breaking it down into parts as you propose. Maybe a few more?

  1. keras-tuner
  2. keras-tuner-tensorflow
  3. keras-tuner-bayesian
  4. keras-tuner-all

if make sense?

DManowitz commented 2 months ago

Or https://github.com/conda-forge/black-feedstock/blob/main/recipe/meta.yaml via black-jupyter, @DManowitz I would consider breaking it down into parts as you propose. Maybe a few more?

1. `keras-tuner`

2. `keras-tuner-tensorflow`

3. `keras-tuner-bayesian`

4. `keras-tuner-all`

if make sense?

@Anselmoo At this point, I think we should keep tensorflow out, since Keras 3.0 can support PyTorch or JAX backends instead of Tensorflow. This package does not need to be the only package installed by someone into their environment. Users can specify which backend(s) they want to use separately.