Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BE] Unify buffer_size across datapipes #335

Open
pmeier opened this issue Mar 28, 2022 · 8 comments · May be fixed by #1077
Open

[BE] Unify buffer_size across datapipes #335

pmeier opened this issue Mar 28, 2022 · 8 comments · May be fixed by #1077
Labels
Better Engineering

Comments

@pmeier
Copy link
Contributor

pmeier commented Mar 28, 2022

The buffer_size parameter is currently fairly inconsistent across datapipes:

name default buffer_size infinite buffer_size warn on infinite
Demultiplexer 1e3 -1 yes
Forker 1e3 -1 yes
Grouper 1e4 N/A N/A
Shuffler 1e4 N/A N/A
MaxTokenBucketizer 1e3 N/A N/A
UnZipper 1e3 -1 yes
IterKeyZipper 1e4 None no

Here are my suggestion on how to unify this:

  • Use the same default buffer_size everywhere. It makes little difference whether we use 1e3 or 1e4 given that it is tightly coupled with the data we know nothing about. Given today's hardware / datasets, I would go with 1e4, but no strong opinion.
  • Give every datapipe with buffer the ability for an infinite buffer. Otherwise users will just be annoyed and use a workaround. For example, torchvision simply uses INFINITE_BUFFER_SIZE = 1_000_000_000, which for all intents and purposes lives up to its name. Which sentinel we use, i.e. -1 or None, again makes little difference. I personally would use None to have a clear separation, but again no strong opinion other than being consistent.
  • Do not warn on infinite buffer sizes. Especially since infinite buffer is not the default behavior, the user is expected to know what they are doing when setting buffer_size=None. I'm all for having a warning like this in the documentation, but I'm strongly against a runtime warning. For example, torchvision datasets need to use an infinite buffer everywhere. Thus, by using the infinite buffer sentinel, users would always get runtime warnings although neither them nor we did anything wrong.
@pmeier pmeier mentioned this issue Mar 30, 2022
Merged
@pmeier
Copy link
Contributor Author

pmeier commented Apr 5, 2022

@NivekT does the 👍 mean you agree with my suggestions and I can send a PR?

@ejguan
Copy link
Contributor

ejguan commented Apr 5, 2022

@pmeier Sorry I missed this issue before. We are currently exploring a plan to add a BufferSpec class (configuration) for buffer. We should provide an instance for default value of all DataPipe. And, the infinite_buffer should be another special instance of such class. WDYT about this idea

@pmeier
Copy link
Contributor Author

pmeier commented Apr 5, 2022

We are currently exploring a plan to add a BufferSpec class (configuration) for buffer

Is there a design document / issue for this? If not, what would a an instance of this class contain?

@ejguan
Copy link
Contributor

ejguan commented Apr 5, 2022

We are currently exploring a plan to add a BufferSpec class (configuration) for buffer

Is there a design document / issue for this? If not, what would a an instance of this class contain?

Not for now. This is a requirement from internal usage that they want to provide some special buffer instance because they would be able to replace a certain instance of DataPipe in the graph but using the BufferSpec instance. For OSS side, this class is really straight forward.

class BufferSpec:
    buffer_size: int

If users use an integer as an argument, we should automatically change it into an instance of BufferSpec.

@pmeier
Copy link
Contributor Author

pmeier commented Apr 5, 2022

I can't comment on the internal use case, but if the public API still accepts int's and any sentinel for the infinite buffer, I'm ok with that.

@ejguan
Copy link
Contributor

ejguan commented Apr 5, 2022

Yeah. And, for this issue, we should definitely unify the buffer_size.
For the infinite one, I agree None is better than -1. WDYT whether or not we should raise Error for negative value. If not raising value, we are kind supporting both sentinels for infinite.

@pmeier
Copy link
Contributor Author

pmeier commented Apr 5, 2022

WDYT whether or not we should raise Error for negative value. If not raising value, we are kind supporting both sentinels for infinite.

Let's not have two ways to do the same thing. If we decide on a sentinel, everything else should error if not isinstance(buffer_size, int) and buffer_size > 1.

@ejguan
Copy link
Contributor

ejguan commented Apr 5, 2022

Makes sense to me

@VitalyFedyunin VitalyFedyunin changed the title Unify buffer_size across datapipes [BE] Unify buffer_size across datapipes Jul 6, 2022
alexanderbattig added a commit to alexanderbattig/data that referenced this issue Mar 11, 2023
@alexanderbattig
Unify buffer_size, following the convention suggested in pytorch#335
90aa8bd
alexanderbattig added a commit to alexanderbattig/pytorch that referenced this issue Mar 11, 2023
@alexanderbattig
Unify buffer_size, following the convention suggested in pytorch/data…
c3c7f5e 
…#335
@ejguan ejguan linked a pull request Mar 13, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Better Engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Unify buffer_size across TorchData DataPipes alexanderbattig/data
3 participants
@VitalyFedyunin @pmeier @ejguan