Fix: Dataset infos() can be broken if a transform not redefining infos() is stacked on the top

[vinnamkim]

Summary

• Ticket no. 115725
• Fix: Dataset infos() can be broken if a transform not redefining infos() is stacked on the top
• Enhance the StreamDatasetStorage transform tests added in Implement StreamDatasetStorage and StreamDataset #1077.
• Test call_count as well in the tests to validate stacked transforms.

How to test

Checklist

• I have added unit tests to cover my changes.​
• I have added integration tests to cover my changes.​
• I have added the description of my changes into CHANGELOG.​
• I have updated the documentation accordingly

License

• I submit my code changes under the same MIT License that covers the project.
  Feel free to contact the maintainers if that's a concern.
• I have updated the license header for each file (see an example below).

# Copyright (C) 2023 Intel Corporation
#
# SPDX-License-Identifier: MIT

[wonjuleee]

Don't we need to have DatasetStorageTest to compare them?

[vinnamkim]

Don't we need to have DatasetStorageTest to compare them?

tests/unit/components/test_dataset_storage.py has the tests to validate StreamDatasetStorage behave as we expect. DatasetStorage has been used as the core of other components, so that our many existing tests have been tested DatasetStorage I think.

[wonjuleee]

Don't we need to have DatasetStorageTest to compare them?

tests/unit/components/test_dataset_storage.py has the tests to validate StreamDatasetStorage behave as we expect. DatasetStorage has been used as the core of other components, so that our many existing tests have been tested DatasetStorage I think.

Partially agree. In my opinion, it was definitely tested in terms of functional, but it was not for performance. Your test for StreamDatasetStorage looks like related to performance (how many times an iterator called) and how it operated internally. If you agree, let me prepare the tests for DatasetStorage with similar shape in another PR. Please give me your opinion on this.

Moreover, we may need to evaluate the quality of operations in terms of time and memory consumption in the future.

[vinnamkim]

Don't we need to have DatasetStorageTest to compare them?

tests/unit/components/test_dataset_storage.py has the tests to validate StreamDatasetStorage behave as we expect. DatasetStorage has been used as the core of other components, so that our many existing tests have been tested DatasetStorage I think.

Partially agree. In my opinion, it was definitely tested in terms of functional, but it was not for performance. Your test for StreamDatasetStorage looks like related to performance (how many times an iterator called) and how it operated internally. If you agree, let me prepare the tests for DatasetStorage with similar shape in another PR. Please give me your opinion on this.

Moreover, we may need to evaluate the quality of operations in terms of time and memory consumption in the future.

No, tests/unit/components/test_dataset_storage.py was 100% designed for the functional. The reason why we confirm how many the __iter__() of extractor is called is that we want to confirm that StreamDatasetStorage behaves in a stream manner (to test its functional, not performance). If it is DatasetStorage, __iter__().call_count always remains one although we stack any number of transforms (because of automatic caching).

To test the performance, I think that the unit test is not right place for it. Unit tests are aim to test the functionality of small components as possible. Therefore, there should exist pass or fail always. However, the performance test can be easily flaky by many variable factors (machine status, network status, ...). In my opinion, performance tests usually are conducted periodically and produce a report rather than testing pass/fail like as we do in NNCF.

[wonjuleee]

Don't we need to have DatasetStorageTest to compare them?

tests/unit/components/test_dataset_storage.py has the tests to validate StreamDatasetStorage behave as we expect. DatasetStorage has been used as the core of other components, so that our many existing tests have been tested DatasetStorage I think.

Partially agree. In my opinion, it was definitely tested in terms of functional, but it was not for performance. Your test for StreamDatasetStorage looks like related to performance (how many times an iterator called) and how it operated internally. If you agree, let me prepare the tests for DatasetStorage with similar shape in another PR. Please give me your opinion on this.
Moreover, we may need to evaluate the quality of operations in terms of time and memory consumption in the future.

No, tests/unit/components/test_dataset_storage.py was 100% designed for the functional. The reason why we confirm how many the __iter__() of extractor is called is that we want to confirm that StreamDatasetStorage behaves in a stream manner (to test its functional, not performance). If it is DatasetStorage, __iter__().call_count always remains one although we stack any number of transforms (because of automatic caching).

To test the performance, I think that the unit test is not right place for it. Unit tests are aim to test the functionality of small components as possible. Therefore, there should exist pass or fail always. However, the performance test can be easily flaky by many variable factors (machine status, network status, ...). In my opinion, performance tests usually are conducted periodically and produce a report rather than testing pass/fail like as we do in NNCF.

I don't think it is functional. I think we should run StreamDatasetStorage with other components (such as merge, transform, etc) and should check the same expected results for the functional tests.

[vinnamkim]

I don't think it is functional. I think we should run StreamDatasetStorage with other components (such as merge, transform, etc) and should check the same expected results for the functional tests.

We exactly have been doing that by parametrizing Dataset and StreamDataset for the importer and exporter tests (yes, not for merge, but transform is did in this test, tests/unit/components/test_dataset_storage.py, but for the DatasetStorage level, not for Dataset level still). However, I mean that this test is aimed to prove it's core mechanism (no caching and streaming) and behaviors.