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

Made changes to some errors prone lines #272

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shyam314159
Copy link

  1. Added code in train.py so one can load the model if it already exists and continue training.
  2. Added .keras file extension so it shows no error while saving the model or loading the model.

Copy link
Collaborator

@kralka kralka left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

save_best_only=True),
TensorBoard(log_dir="logs/" + stub, update_freq="batch")
]

if os.path.exists(file_path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check needed? It seems more intuitive to "start training again" than to "train a certain number of epochs more".

Copy link
Author

Choose a reason for hiding this comment

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

It's indeed used for "start training again" as an individual I have no computational resources to continue 75 epochs in single go this will be helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see.

Are we talking about the tinyAES dataset or ASCADv2? Because the tinyAES should be quite fast even on a moderate computer without a GPU (using the sample configs). Also there are fewer epochs in the configs. In case of ASCADv2 please use the GPAM model (https://github.com/google/scaaml/tree/main/papers/2024/GPAM#models) since I doubt the initial model is powerful enough for this dataset.

tinyAES: There is a possibility to use https://colab.google/ (something like jupyterlab running on cloud). One can request a GPU or a TPU (K8 IIRC) and run for a limited time (8 hours IIRC). The problem is that the saved files count into your Google account quota. This might be ok for the case of tinyAES but ASCADv2 is larger than the free quota (unprocessed). I never tried mounting a Google cloud bucket into a colab (could be very slow, downloaded data might be limited).

As far as the code change goes: if you find the feature useful I would prefer adding an argument allow_model_reload to train_model and omit the print statements. And then a command line argument defaulting to false: https://stackoverflow.com/questions/15008758/parsing-boolean-values-with-argparse

Copy link
Author

Choose a reason for hiding this comment

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

yes I was talking about ASCAD dataset

stub = get_model_stub(attack_point, attack_byte, config)
file_path = f"models/{stub}.keras"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This modernises the code. However we would be silently losing backwards compatibility with pretrained models. Since this code is for the 2019 Defcon presentation I would prefer to pin a TensorFlow version here (say 2.12) and have more up to date code for the more recent projects. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

you're right back compatibility is a concern but when I tried to run the code it threw an error asking to add a keras extension so I did. I was using the latest version of everything.

Copy link
Author

Choose a reason for hiding this comment

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

ok this might not be an issue if we install the version mentioned in requirements.

@kralka
Copy link
Collaborator

kralka commented Sep 10, 2024

And sorry the reply took me so long, we were quite busy last week.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10630507131

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.4%

Totals Coverage Status
Change from base Build 10661092540: 0.0%
Covered Lines: 2252
Relevant Lines: 2801

💛 - Coveralls

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