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

Incorrect dependencies #35

Open
1 task done
DManowitz opened this issue Jun 28, 2024 · 7 comments · May be fixed by #36
Open
1 task done

Incorrect dependencies #35

DManowitz opened this issue Jun 28, 2024 · 7 comments · May be fixed by #36
Labels
bug Something isn't working

Comments

@DManowitz
Copy link

Solution to issue cannot be found in the documentation.

  • I checked 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
@DManowitz DManowitz added the bug Something isn't working label Jun 28, 2024
@Anselmoo
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

DManowitz commented Jun 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants