-
Notifications
You must be signed in to change notification settings - Fork 609
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
Add warnings against resetting pipeline before end of epoch and test parallel ES with fw iterator #4023
base: main
Are you sure you want to change the base?
Add warnings against resetting pipeline before end of epoch and test parallel ES with fw iterator #4023
Changes from 6 commits
b4e47b7
2ed59da
c58c699
1d1bd8b
f2ea23b
cd233bb
b682272
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -920,6 +920,14 @@ def schedule_run(self): | |
Should not be mixed with :meth:`run` in the same pipeline""" | ||
with self._check_api_type_scope(types.PipelineAPIType.SCHEDULED): | ||
if self._first_iter and self._exec_pipelined: | ||
# For one, when prefetching, parallel external source will reuse buffers | ||
# that might be still referenced by no_copy input fed to pipeline | ||
if not self.empty(): | ||
warnings.warn( | ||
"Prefetching data into a non-empty pipeline may result in corrupted " | ||
"outputs. Please make sure all batches from previous epoch are consumed " | ||
"before scheduling work for a new epoch.", | ||
Warning) | ||
self._prefetch() | ||
else: | ||
self._run_once() | ||
|
@@ -1078,7 +1086,19 @@ def reset(self): | |
|
||
If pipeline iterator reached the end then reset its state to the beginning. | ||
""" | ||
if self._last_iter: | ||
if not self._last_iter: | ||
# resetting before some external source raised StopIteration is a no-op | ||
if self._input_callbacks: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How legitimate use case is pipeline with external source that is infinite + FW iterator with iterator size passed explicitly? Because in that case, the warning will be triggered too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 I guess it still can happen and we should behave consistently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😟I thought that one would be useful if one has two or more pipelines with (P)ES that raise StopIteration and should be reset but for some reason the epochs diverge. Either because the number of iterations is really (but unintentionally) different in those two or because one uses .run API, with prefetch_queue_depth 1 and resets all pipelines when the first one raises, not letting the others actually reach end of epoch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I could safegaurd this check with pipeline._epoch_idx> 0. It seems that if it has ever been incremented, then there must be ES that raises StopIteration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that we should warn that something may be not right and if te user provides |
||
warnings.warn( | ||
"Resetting the pipeline before any of the external sources reached " | ||
"the end of epoch (i.e. raised StopIteration) has no effect.", | ||
Warning) | ||
else: | ||
if not self.empty(): | ||
warnings.warn( | ||
"Resetting the pipeline before all scheduled batches have been consumed " | ||
"is discouraged and may be unsupported in the future.", | ||
Warning) | ||
self._first_iter = True | ||
self._last_iter = False | ||
self._iter = 0 | ||
|
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.
I think it is possible, prefetch queue depth is 2, batches to consume is 1, we can still schedule one more run. The native part can overschedule - it will just wait for the empty output buffer, but the ES may fail (parallel mode with nocopy, but the regular ES does copy and have an internal queue).
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.
Do you mean that it should be supported? By possible you mean that using fw iterators correctly you may still end up in such situation?
If that should be supported:
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.
I think so. You can still use this API without the FW iterator.