Fix configuration of a custom serializers for one of the predefined types

[enrico-stauss]

Before submitting

• Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

What does this PR do?

This is only a suggestion. When experimenting with litdata I tried to fix the no_header_tensor serializer issue by replacing it with a custom subclass. I noticed that my custom subclass was not applied and tracked it down to the modified bit.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[tchaton]

Hey @enrico-stauss. Thanks for your contribution :) Would you mind adding a test ?

[enrico-stauss]

Hey @enrico-stauss. Thanks for your contribution :) Would you mind adding a test ?

Hi @tchaton, I changed my modification s.t. it preserves the original order of the serializers apart from placing explicitly specified serializers in the beginning. Now the test that failed should work. Also I implemented a test that assures that a custom implementation for a predefined serializer type is preferred over the default one.

I have to note however, that the use of an OrderedDict and relying on the oder of the serializers seems to be quite error-prone. Anyway I hope it helps and I appreciate your work!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@7fe1c26). Click here to learn what that means.

Additional details and impacted files

@@          Coverage Diff          @@
##             main   #125   +/-   ##
=====================================
  Coverage        ?    76%           
=====================================
  Files           ?     30           
  Lines           ?   3970           
  Branches        ?      0           
=====================================
  Hits            ?   3015           
  Misses          ?    955           
  Partials        ?      0           

[tchaton]

Hey @enrico-stauss. Thanks for your contribution :) Would you mind adding a test ?

Hi @tchaton, I changed my modification s.t. it preserves the original order of the serializers apart from placing explicitly specified serializers in the beginning. Now the test that failed should work. Also I implemented a test that assures that a custom implementation for a predefined serializer type is preferred over the default one.

I have to note however, that the use of an OrderedDict and relying on the oder of the serializers seems to be quite error-prone. Anyway I hope it helps and I appreciate your work!

Yeah, I know. We are trying to be smart to reduce the user overhead, but this leads to the annoyance of this ordered serialiser list where we check types against them.

There is probably a better design to auto-infer the format but I didn't have time to look into it. Feel free to give it a try, pretty sure it is a fun problem to solve.

[enrico-stauss]

Hey @enrico-stauss. Thanks for your contribution :) Would you mind adding a test ?

Hi @tchaton, I changed my modification s.t. it preserves the original order of the serializers apart from placing explicitly specified serializers in the beginning. Now the test that failed should work. Also I implemented a test that assures that a custom implementation for a predefined serializer type is preferred over the default one.
I have to note however, that the use of an OrderedDict and relying on the oder of the serializers seems to be quite error-prone. Anyway I hope it helps and I appreciate your work!

Yeah, I know. We are trying to be smart to reduce the user overhead, but this leads to the annoyance of this ordered serialiser list where we check types against them.

There is probably a better design to auto-infer the format but I didn't have time to look into it. Feel free to give it a try, pretty sure it is a fun problem to solve.

Hi @tchaton,
I have been thinking about getting more involved with the project but unfortunately my time is quite limited at the moment. I barely find time to make progress on my own projects. Maybe I'll come back to litdata when my schedule has freed up a bit. Keep it up :)

[tchaton]

k to litdata when my schedule has freed up a bit. Keep it up :)

Thanks @enrico-stauss for contributing already.