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

keras.src vs keras.api design question #20420

Closed
adrinjalali opened this issue Oct 28, 2024 · 6 comments
Closed

keras.src vs keras.api design question #20420

adrinjalali opened this issue Oct 28, 2024 · 6 comments
Assignees
Labels
type:support User is asking for help / asking an implementation question. Stackoverflow would be better suited.

Comments

@adrinjalali
Copy link
Contributor

This is more of a question for me to better understand the codebase.

Working on #20399 , I realised since there's a distinction between keras.src and keras.api (which is exposed as keras in the end), makes it impossible to do certain things.

For instance, if you want to typehint an input as keras.Model, then you'd need to do a import keras.Model kinda thing. But that results in a circular import issue along these lines:

Cell In[1], line 1
----> 1 from keras.wrappers import KerasClassifier

File ~/Projects/gh/me/keras/keras/__init__.py:4
      1 import os
      3 # DO NOT EDIT. Generated by api_gen.sh
----> 4 from keras.api import DTypePolicy
      5 from keras.api import FloatDTypePolicy
      6 from keras.api import Function

File ~/Projects/gh/me/keras/keras/api/__init__.py:7
      1 """DO NOT EDIT.
      2 
      3 This file was autogenerated. Do not edit it by hand,
      4 since your modifications would be overwritten.
      5 """
----> 7 from keras.api import _tf_keras
      8 from keras.api import activations
      9 from keras.api import applications

File ~/Projects/gh/me/keras/keras/api/_tf_keras/__init__.py:1
----> 1 from keras.api._tf_keras import keras

File ~/Projects/gh/me/keras/keras/api/_tf_keras/keras/__init__.py:28
     26 from keras.api import utils
     27 from keras.api import visualization
---> 28 from keras.api import wrappers
     29 from keras.api._tf_keras.keras import backend
     30 from keras.api._tf_keras.keras import layers

File ~/Projects/gh/me/keras/keras/api/wrappers/__init__.py:7
      1 """DO NOT EDIT.
      2 
      3 This file was autogenerated. Do not edit it by hand,
      4 since your modifications would be overwritten.
      5 """
----> 7 from keras.src.wrappers._sklearn import KerasClassifier
      8 from keras.src.wrappers._sklearn import KerasRegressor

File ~/Projects/gh/me/keras/keras/src/wrappers/_sklearn.py:37
     35 import keras
     36 from keras.src import losses as losses_module
---> 37 from keras import Model
     38 from keras.src.api_export import keras_export
     39 from keras.src.wrappers._utils import accepts_kwargs

ImportError: cannot import name 'Model' from partially initialized module 'keras' (most likely due to a circular import) (/home/adrin/Projects/gh/me/keras/keras/__init__.py)

Checking the codebase, I realise typehints are not a thing we do here, so I'll remove them, but it still begs the question, what are the gains with the separation of the two folders, which adds quite a bit of complexity. In other projects, we tend to have a leading _ on file names, and __init__.py exposes what needs to be public on the user API level.

@mehtamansi29
Copy link
Collaborator

Hi @adrinjalali -

Here keras.src is for implementing core functionality of layers, activations etc. While keras.api is seems to be public API use by general users. For importing keras model here you can use from keras.models import Sequential,Model instead of `import keras.Model' in latest keras3.

@mehtamansi29 mehtamansi29 added type:support User is asking for help / asking an implementation question. Stackoverflow would be better suited. stat:awaiting response from contributor labels Oct 28, 2024
@adrinjalali
Copy link
Contributor Author

The issue is not really the difference between keras.models.Model and keras.Model here, since both paths are exported anyway:

@keras_export(["keras.Model", "keras.models.Model"])
class Model(Trainer, base_trainer.Trainer, Layer):

Changing the import as you mention, doesn't solve the issue and results in the same import recursion error.

Note that for the user, this is not an issue, they simply import form thte public API, which is keras.not-src*. However, when you develop inside keras, then you can't really import from keras.not-src.

Also, IIUC, keras.api is not something maintained manually / different from keras.src. The api_gen.py simply looks for all cases of @keras_export and creates __init__.py files under keras.api (and for some reason keras.api._tf_keras) importing them, and then the installation exposes that folder as keras. Oddly enough, it assumes the folder structure exists, and only overwrites the __init__.py files, and it'd fail if a folder under keras.api is missing.

All of this seems rather overcomplicating the codebase / release provess, and my question is the gain we get from having this process.

I'm also not sure how a typehint would render if keras were to accept them, since the import paths of the objects cannot be the same path as what users import, and that's how I got to this issue.

@fchollet
Copy link
Collaborator

If you are in the Keras codebase, you can only import symbols from keras.src. So to add type annotations for Model, you would simply import e.g. from keras.src.models import Model. Note that in practice the Keras codebase does not use any type annotations -- which is more Pythonic.

@adrinjalali
Copy link
Contributor Author

Thanks, yes I realised all that. But it doesn't really answer the question as, why the two folders are separate though.

@fchollet
Copy link
Collaborator

why the two folders are separate though

Because this lets us define the public API surface separately from the implementation. Exposing all of your internal logic to the user is quite bad practice. We want public APIs to be opt-in only. It's unfortunate that Python packaging does not provide a built-in feature to achieve this.

Copy link

Are you satisfied with the resolution of your issue?
Yes
No

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:support User is asking for help / asking an implementation question. Stackoverflow would be better suited.
Projects
None yet
Development

No branches or pull requests

3 participants