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

Updated Keras and TensorFlow. #1174

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from

Conversation

tanwarsh
Copy link
Collaborator

@tanwarsh tanwarsh commented Nov 26, 2024

Related issue #973.

Changes in this PR:

  1. Updated Tensorflow to Tensorflow v2.18.0 and Keras 3
  2. Updated openfl/federated/task/runner_kr.py to handle model and optimizer weights.
  3. Updated Keras_cnn_mnist workflow.
  4. Update optimizer.
  5. Standardizing the workspace.

Workspaces:

  1. Keras_cnn_mnist - working
  2. Keras_cnn_with_compression - WIP
  3. Keras_nlp - working

This PR will be merged after all the workspaces mentioned above are working.

@tanwarsh tanwarsh changed the title Updated Keras and TensorFlow. WIP:Updated Keras and TensorFlow. Nov 26, 2024
@tanwarsh tanwarsh marked this pull request as draft November 26, 2024 13:08
@tanwarsh tanwarsh marked this pull request as ready for review November 27, 2024 06:01
@tanwarsh tanwarsh changed the title WIP:Updated Keras and TensorFlow. Updated Keras and TensorFlow. Nov 27, 2024
@tanwarsh
Copy link
Collaborator Author

I will add code changes for the remaining Keras workspace - Keras_cnn_with_compression and keras_nlp.

@teoparvanov @kta-intel @psfoley, I was facing an issue with optimizer weights due to resetting the weights before saving them to the tensor database and using them to rebuild the model, which resulted in no improvement in accuracy. I have skipped the rebuild (and hence resetting the model optimizer weights) for the first round. This can be further discussed, and we can proceed with these changes once all the pipelines are working and the workspace changes are also completed.

Copy link
Collaborator

@teoparvanov teoparvanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. A couple of questions came up during my read-through:

.github/workflows/ubuntu.yml Show resolved Hide resolved
@@ -60,7 +60,7 @@ def rebuild_model(self, round_num, input_tensor_dict, validation=False):
"""
if self.opt_treatment == "RESET":
self.reset_opt_vars()
self.set_tensor_dict(input_tensor_dict, with_opt_vars=False)
self.set_tensor_dict(input_tensor_dict, with_opt_vars=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, if the opt treatment is "RESET", shouldn't we have with_opt_vars=False ?

Copy link
Collaborator Author

@tanwarsh tanwarsh Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done as when optmizers weights are reset. Leraning rate is zero and model accuracy remiains around ~0.125.

openfl/federated/task/runner_keras.py Show resolved Hide resolved
openfl/federated/task/runner_keras.py Outdated Show resolved Hide resolved
openfl-workspace/keras_nlp/requirements.txt Outdated Show resolved Hide resolved
@@ -22,10 +22,10 @@ jobs:

steps:
- uses: actions/checkout@v3
- name: Set up Python 3.8
- name: Set up Python 3.9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the task_runner_e2e.yaml workflow as well. It runs nightly so not failed for the PR.

@@ -22,10 +22,10 @@ jobs:

steps:
- uses: actions/checkout@v3
- name: Set up Python 3.8
- name: Set up Python 3.9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 3.8 not supported anymore, change the README documents as well for the same.

from tensorflow.keras.layers import Conv2D
from tensorflow.keras.layers import Dense
from tensorflow.keras.layers import Flatten
from keras.models import Sequential
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tensorflow Keras is still evident and we see many examples in the ML community for the same.
Shouldn't we add separate workspace for Keras 3.0 for now and remove this eventually.

Or we are saying that we are not going to support tensorflow keras anymore.

@@ -17,8 +18,8 @@

with catch_warnings():
simplefilter(action="ignore")
import keras as ke
import tensorflow as tf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally tensorflow must not be used with Keras as Keras implementation must be free of any backend while running.
Please find the alternative for the same.

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.

3 participants