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

Update URL config protocol for loading TF models #1505

Merged
merged 31 commits into from
Feb 3, 2025
Merged

Conversation

reallsx
Copy link
Contributor

@reallsx reallsx commented Dec 20, 2024

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What does the code in this PR do / what does it improve?

Make the loading of tensorflow model (for position reconstruction) compatible with the newer versions of tensorflow.

Can you briefly describe how it works?

  1. If the file ends with .keras, load the file directly with tf.keras.models.load_model.
  2. If the file ends with .tar.gz, extract the tarball. Then first execute the registration.py in it, then load the .keras file in it with tf.keras.models.load_model.

Can you give a minimal working example (or illustrate with a figure)?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.
  • On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

All italic comments can be removed from this template.

@reallsx reallsx requested a review from dachengx January 7, 2025 20:13
@reallsx
Copy link
Contributor Author

reallsx commented Jan 15, 2025

Seems like the tests failed because keras is not able to load the model...

While this PR is for changing the protocol for loading models for TF>2.18 which comes with keras 3 the testing environment is still using TF 2.15 so I guess this is why it is failing.

Collecting tensorflow==2.15.0.post1 (from base_environment==2024.1.1)

@reallsx reallsx marked this pull request as ready for review January 15, 2025 23:11
@dachengx
Copy link
Collaborator

dachengx commented Jan 16, 2025

Seems like the tests failed because keras is not able to load the model...

While this PR is for changing the protocol for loading models for TF>2.18 which comes with keras 3 the testing environment is still using TF 2.15 so I guess this is why it is failing.

Collecting tensorflow==2.15.0.post1 (from base_environment==2024.1.1)

We can try to update it to tensorflow>2.18, after

@LuisSanchez25 @rsriya @ghusheng @GiovanniVolta

@yuema137 yuema137 self-requested a review January 21, 2025 22:56
@dachengx
Copy link
Collaborator

Is it normal to see warning

/home/xudc/.local/lib/python3.9/site-packages/keras/src/saving/saving_lib.py:757: UserWarning: Skipping variable loading for optimizer 'adam', because it has 18 variables whereas the saved optimizer has 2 variables. 
  saveable.load_own_variables(weights_store.get(inner_path))
<Sequential name=sequential, built=True>

when loading https://github.com/XENONnT/corrections/blob/3e8727006511081fc2cc6bd4c5948eba673d65d8/XENONnT/position_reconstruction/data/v9.json#L46C19-L46C47?

@dachengx dachengx mentioned this pull request Jan 24, 2025
@reallsx
Copy link
Contributor Author

reallsx commented Jan 24, 2025

Is it normal to see warning

/home/xudc/.local/lib/python3.9/site-packages/keras/src/saving/saving_lib.py:757: UserWarning: Skipping variable loading for optimizer 'adam', because it has 18 variables whereas the saved optimizer has 2 variables. 
  saveable.load_own_variables(weights_store.get(inner_path))
<Sequential name=sequential, built=True>

when loading https://github.com/XENONnT/corrections/blob/3e8727006511081fc2cc6bd4c5948eba673d65d8/XENONnT/position_reconstruction/data/v9.json#L46C19-L46C47?

This happens in my environment as well and the warning appears only once in a session. The warning is about the optimizer which has nothing to do when loading the model for inference. I think it is fine.

@coveralls
Copy link

Coverage Status

coverage: 89.688% (-0.3%) from 89.966%
when pulling 4f3e41d on tf_protocal
into 977c2ea on master.

@dachengx dachengx merged commit 099475a into master Feb 3, 2025
7 checks passed
@dachengx dachengx deleted the tf_protocal branch February 3, 2025 05:19
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.

5 participants