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

Porting text Preprocessing from tf.keras to keras_core #672

Closed
wants to merge 1 commit into from

Conversation

asingh9530
Copy link
Contributor

@asingh9530 asingh9530 commented Aug 7, 2023

Hi @fchollet ,

This PR is specifically for porting Text-Preprocessing layer from tf.keras to keras_core.

Note not yet completed.

Issue : keras-team/keras#18421

Thanks

@asingh9530
Copy link
Contributor Author

asingh9530 commented Aug 8, 2023

Hi @fchollet,

Need confirmation from you following needs to be ported.

  • DataHandler as adapt method will require it, so need your confirmation since in keras_core we don't have engine how about we use utils instead.
  • Also since textVectorization depends on StringLookup so it is needed to migrate StringLookup also in this PR.

@fchollet
Copy link
Member

fchollet commented Aug 8, 2023

Thanks for the PR. I took a close look at the contents, and unfortunately there's a huge amount of things that would need to be removed or refactored. The problem is that this is not meant to be a 1:1 copy of the code in tf.keras. There's a significant amount of adaption that's needed. E.g. tf.__internal__.monitoring.BoolGauge is not needed, references to tf.compat.v1 need to be removed, error messages must be adapted to refer to Keras Core, etc.

I appreciate your efforts to make a contribution, but I'm afraid this item is not turning out to be a good match. You could take on another item instead: keras-team/keras#18442

@fchollet fchollet closed this Aug 8, 2023
@asingh9530
Copy link
Contributor Author

Thanks for the PR. I took a close look at the contents, and unfortunately there's a huge amount of things that would need to be removed or refactored. The problem is that this is not meant to be a 1:1 copy of the code in tf.keras. There's a significant amount of adaption that's needed. E.g. tf.__internal__.monitoring.BoolGauge is not needed, references to tf.compat.v1 need to be removed, error messages must be adapted to refer to Keras Core, etc.

I appreciate your efforts to make a contribution, but I'm afraid this item is not turning out to be a good match. You could take on another item instead: keras-team/keras#18442

Hi @fchollet, Yes I agree this PR will require a big change and refactoring. I wanted to know do you have any RFC or document for keras_core as it will be easier to make large changes which might require structural changes or large refactoring. If it is something internal then its fine otherwise it will be very helpful. 🙏🏻

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.

2 participants