Add CLI script exporting CLIP Toy model IREE test data #672
Conversation
|
This PR requires that #664 is merged first. |
sogartar
force-pushed
the
clip-text-model-export-toy-iree-test-data
branch
from
fc67e1a to
a2ea0c2
Compare
This is required to have an easy way of exporting test data that will be used in IREE to guard against regressions. E.g. ``` python -m sharktank.models.clip.export_toy_text_model_iree_test_data \ --output-path-prefix=clip_toy_text_model ``` Refactor some of the existing tests to reuse the new export logic.
sogartar
force-pushed
the
clip-text-model-export-toy-iree-test-data
branch
from
a2ea0c2 to
b71e906
Compare
sogartar
changed the title
| @@ -67,7 +72,7 @@ def export_clip_text_model_dataset_from_hugging_face( | |||
|
|
|||
|
|
|||
| def export_clip_text_model_mlir( | |||
| model: Union[ClipTextModel, PathLike], | |||
| model: Union[ClipTextModel, AnyPath], | |||
There was a problem hiding this comment.
Leave this as PathLike and remove the AnyPath. Users should just make sure to convert the string before invoking. (Same as above)
There was a problem hiding this comment.
I removed AnyPath.
| input_ids=input_ids, | ||
| target_iree_parameters_output_path=target_iree_parameters_output_path, | ||
| target_mlir_output_path=target_mlir_output_path, | ||
| input_ids_output_path=input_ids_output_path if i == 0 else None, |
There was a problem hiding this comment.
Split this across lines or put in an if conditional. The , between make the behavior extremely ambiguous.
| target_iree_parameters_output_path=target_iree_parameters_output_path, | ||
| target_mlir_output_path=target_mlir_output_path, | ||
| input_ids_output_path=input_ids_output_path if i == 0 else None, | ||
| expected_last_hidden_state_output_path=expected_last_hidden_state_output_path |
There was a problem hiding this comment.
Same comment as above. This looks like it should just be in an if condition rather than a oneliner
| def testSmokeExportToyIreeTestData(self): | ||
| from sharktank.models.clip.export_toy_text_model_iree_test_data import main | ||
|
|
||
| main([f"--output-path-prefix={self.path_prefix}clip_toy_text_model"]) |
There was a problem hiding this comment.
Use pythons path library to operate on this instead of string manipulations.
There was a problem hiding this comment.
Done. I changed it to use a directory as a prefix.
| if ( | ||
| not self.caching | ||
| or not os.path.exists(mlir_path) | ||
| or not os.path.exists(parameters_path) |
There was a problem hiding this comment.
This conditional feels weird / wrong. Is it checking for defaults? Also it feels particularly weird for a test. Reconsider why this is needed.
There was a problem hiding this comment.
I removed the caching. It is useful to speedup iteration, but it does add clutter.
This is required to have an easy way of exporting test data that will be
used in IREE to guard against regressions.
E.g.
Refactor some of the existing tests to reuse the new export logic.