-
Notifications
You must be signed in to change notification settings - Fork 855
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
Train and inference with different image data types #2535
Conversation
2716445
to
a182803
Compare
@@ -131,6 +131,33 @@ def is_rois_column(data: pd.Series) -> bool: | |||
return is_rois_input(data[idx]) | |||
|
|||
|
|||
def is_image_path(feature: Any): | |||
is_path = True | |||
image_paths = str(feature).split(";") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why we need to split by ";"? Is there any upstream logics introducing it or some assumptions made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there is an existing upstream logic for image_path
that we use ";" to separate a list of images.
def _is_image_path(self, feature: Any): | ||
is_path = True | ||
image_paths = str(feature).split(";") | ||
for img_path in image_paths: | ||
try: | ||
with PIL.Image.open(img_path) as img: | ||
pass | ||
break | ||
except: | ||
is_path = False | ||
return is_path | ||
|
||
def _is_image_bytearray(self, feature: Any): | ||
is_bytearray = True | ||
if not isinstance(feature, list): | ||
feature = [feature] | ||
for img_bytearray in feature: | ||
try: | ||
with PIL.Image.open(BytesIO(img_bytearray)) as img: | ||
pass | ||
break | ||
except: | ||
is_bytearray = False | ||
return is_bytearray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 functions seems to be duplicate of what defined in infer_types.py
. Better to consolidate and reuse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the intention was to add these utility function inside./multimodal/data/utils.py
or ./multimodal/utils/data.py
, but they both depend on MultiModalFeaturePreprocessor
which creates circular dependency. Unless we want to create a new utility function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we let infer_types.py
use the function defined here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require us to initialize a MultiModalFeaturePreprocessor
with variables that are not readily available, and potentially introduce unstable dependencies since there are files that import both infer_types
and MultiModalFeaturePreprocessor
, e.g.
autogluon/multimodal/src/autogluon/multimodal/predictor.py
Lines 86 to 92 in 0a62d48
from .data.infer_types import ( | |
infer_column_types, | |
infer_label_column_type_by_problem_type, | |
infer_problem_type_output_shape, | |
infer_rois_column_type, | |
) | |
from .data.preprocess_dataframe import MultiModalFeaturePreprocessor |
809eae3
to
acf4464
Compare
acf4464
to
b8a52d8
Compare
data = data.tolist() | ||
image_type = get_image_feature_type(data.iloc[0]) | ||
if image_type == IMAGE_PATH: | ||
data = data.apply(lambda ele: str(ele).split(";")).tolist() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use vectorized form: data.str.split(';').tolist()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to distinguish
image_path
andimage_bytearray
in the column types. These image sub-types would be used in df preprocessor and image processor. The current modification breaks the original logic.The problem is that there is a difference of usage between training and inference time. We can train using images on disk and then use bytearrays during the inference. Saving files to a file system during the inference time creates additional security overhead.
I agree that we need to support using image_path in training and image_bytearray in inference. To do so, we can infer the image column types during inference and see whether the subtype changes. If we detect subtype changes, we can modify the
_column_types
in df preprocessor to reflect them. There is no need to change the internal logic among infer_types, df preprocessor, and processor.
Thanks for the suggestion. I'm working on a quick POC for this idea, and hopefully we can still catch the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to distinguish image_path
and image_bytearray
in the column types. These image sub-types would be used in df preprocessor and image processor. The current modification breaks the original logic.
The problem is that there is a difference of usage between training and inference time. We can train using images on disk and then use bytearrays during the inference. Saving files to a file system during the inference time creates additional security overhead. |
Job PR-2535-b8a52d8 is done. |
I agree that we need to support using image_path in training and image_bytearray in inference. To do so, we can infer the image column types during inference and see whether the subtype changes. If we detect subtype changes, we can modify the |
3af4ed4
to
b16626f
Compare
Job PR-2535-b16626f is done. |
441ce57
to
b240ce3
Compare
Job PR-2535-b240ce3 is done. |
Job PR-2535-441ce57 is done. |
b240ce3
to
8cb0dda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Consider using the updated column_types
to explicitly replace the df_preprocessor._column_types
to make logic more clear.
Wondering what the current behavior would be if the |
437530d
to
a67ff50
Compare
Job PR-2535-8cb0dda is done. |
a67ff50
to
80ce2d9
Compare
80ce2d9
to
725f115
Compare
Job PR-2535-725f115 is done. |
Issue #, if available:
Description of changes:
This PR combines current different image modalities (
image_path
andimage_bytearray
) into singleimage
modality. This supports image training and inference with different feature types.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.