feat: Optimize bytewax pod resource with zero-copy #3826
Conversation
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
from
aac5f12
to
dbf7366
Compare
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
from
dbf7366
to
7dadf56
Compare
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
from
2f17cbf
to
792145c
Compare
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
4 times, most recently
from
c3fca82
to
4eb3209
Compare
| bytewax_replicas: int = 5 | ||
| """ (optional) Number of process to spawn in each pods to handle a file in parallel""" | ||
|
|
||
| bytewax_worker_per_process: int = 1 |
There was a problem hiding this comment.
I don't believe that this setting, and the setting of the number of replicas will currently be respected.
I'd be happy to help with getting this fixed, but perhaps that should be the subject of a separate PR.
There was a problem hiding this comment.
ah yeah you're right, looking forward to working on a separate PR
There was a problem hiding this comment.
I don't believe that this setting, and the setting of the number of replicas will currently be respected.
I'd be happy to help with getting this fixed, but perhaps that should be the subject of a separate PR.
could you point out exactly where the setting is not respected? seems like the env of bytewax pods consume it correctly
There was a problem hiding this comment.
This is where we call the bytewax entrypoint for the dataflow:
which ends up calling:
https://github.com/bytewax/bytewax/blob/v0.15.1/src/execution/mod.rs#L709-L717
Since we aren't using any of the environment variables at this point, we should only ever see one Python process with one worker per pod.
Happy to chat further about enabling multiple workers. Feel free to reach out on slack if you would prefer to do it there.
There was a problem hiding this comment.
Great, would love to chat more, but by the way we're using bytewax 0.17.1 as of now
There was a problem hiding this comment.
by the way, in case the REPLICAS setting doesn't work, we still have at least 1 process and 1 worker per pods.
and the implementation of this PR still reduce memory footprint compares to master's one. should we process this first? @achals
There was a problem hiding this comment.
@sudohainguyen I'd prefer to update the pr so only the changes we expect to actually make a difference are merged (specifically the arrow changes) - we can bump the dependency on bytewax and make any necessary changes for replicas in a separate pr
There was a problem hiding this comment.
agree, then I will narrow down the PR, to make bytewax settings unexposed from batch engine config
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
from
4eb3209
to
bb233cd
Compare
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
sudohainguyen
force-pushed
the
feat/zero-copy-bytewax
branch
from
bb233cd
to
8b4cad2
Compare
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
|
@sudohainguyen are the current tests sufficient to ensure this test is sitting as expected? Can you test some of this manually? |
|
Yep I did, and already applied to my production, that's how I can measure memory usage reduced. |
|
Bumping this out of interest. Hoping it's merged soon. |
|
An amazing improvement! Used this in our workflows. Peak usage was at 512GB before the update. 
|
What this PR does / why we need it: To optimize bytewax pods resource usage, for the case in #3825, each pods only consume 300MB of memory.
Which issue(s) this PR fixes:
Fixes #3825