Add asset zipping functionality to TFJS converter #6915
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @ahmedsabie and @lina128)
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 25 at r1 (raw file):
import shutil import tempfile from zipfile import ZipFile
does the zipfile library have any python version requirement?
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 428 at r1 (raw file):
saved_model.signatures.keys())) def _copy_assets(saved_model_dir, output_dir):
should this function only trigger if the user specify the need to copy assets? (i.e. tfdf model).
This should be controlled by a parameter on the tfjs-converter command line.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 437 at r1 (raw file):
with ZipFile(zip_path, 'w') as archive: for (input_dir_path, _, file_names) in tf.io.gfile.walk(input_assets_path):
does this copy nested directories too?
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @ahmedsabie)
There was a problem hiding this comment.
Reviewable status:
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 25 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
does the zipfile library have any python version requirement?
No its builtin
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 428 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
should this function only trigger if the user specify the need to copy assets? (i.e. tfdf model).
This should be controlled by a parameter on the tfjs-converter command line.
Assets are defined as "arbitrary files that are needed for model use". If we don't support the asset then the model wouldn't run anyways so it should never be a useless copy, and I don't think its good user experience for users to have to know that TFDF uses assets under the hood
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 437 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
does this copy nested directories too?
walk is recursive
There was a problem hiding this comment.
Reviewable status:
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 428 at r1 (raw file):
Previously, ahmedsabie wrote…
Assets are defined as "arbitrary files that are needed for model use". If we don't support the asset then the model wouldn't run anyways so it should never be a useless copy, and I don't think its good user experience for users to have to know that TFDF uses assets under the hood
good point, form the convert_tf_save_model() method perspective, it should be a parameter flag, since for non-tfdf model it should not zip the assets.
There was a problem hiding this comment.
Reviewable status:
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 428 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
good point, form the convert_tf_save_model() method perspective, it should be a parameter flag, since for non-tfdf model it should not zip the assets.
How about checking if the model is a TFDF model after loading and if so add the assets? That way no one calling needs to know what the model they are passing in
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 428 at r1 (raw file):
Previously, ahmedsabie wrote…
How about checking if the model is a TFDF model after loading and if so add the assets? That way no one calling needs to know what the model they are passing in
agree, thanks
The asset files are used for the TFDF library. Does not change behavior of existing models.
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is