Fix mutable default arguments and resource leaks#44287
Fix mutable default arguments and resource leaks#44287Rocketknight1 merged 2 commits intohuggingface:mainfrom
Conversation
|
Those file handles will be closed because they're never assigned to a variable, and so will immediately become unreachable and be cleaned up after the loop! |
|
Good point — reverted the file handle changes. The inline |
Rocketknight1
left a comment
Rocketknight1
left a comment
There was a problem hiding this comment.
This change seems good, yes!
Rocketknight1
force-pushed
the
fix/mutable-defaults-and-resource-leaks
branch
from
bd8dd9d to
a816980
Compare
|
Hi @jashshah999 the processor failing test seems specific to this PR. Can you figure out what's going on? |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Mutable defaults (shared state across calls):
- debug_utils.py: DebugUnderflowOverflow trace_batch_nums=[] -> None
- kernel_config.py: KernelConfig kernel_mapping={} -> None
- modeling_idefics.py: freeze_model/freeze_text_layers/freeze_vision_layers
module_exceptions=[] -> ()
- convert_usefulsensors_to_hf.py: _read_h5_weights weights={} -> None
(actively mutated via dict assignment)
Resource leaks (file handles not closed):
- processing_utils.py: open().read() in chat template loading -> context manager
- tokenization_myt5.py: json.load(open()) -> context manager
- cli/serve.py: config_path.open().read() -> context manager
The open().read() patterns without variable assignment are immediately unreachable and cleaned up by CPython's reference counting GC, so context managers are unnecessary here. Keeps the mutable default argument fixes which are real bugs.
Head branch was pushed to by a user without write access
jashshah999
force-pushed
the
fix/mutable-defaults-and-resource-leaks
branch
from
a816980 to
efce737
Compare
|
I ran the idefics processor tests locally and all 22 pass. Rebased on latest main and force pushed -- the CI should re-run now. If it still fails, happy to dig deeper into which specific test is breaking. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: idefics, moonshine |
4 participants
What does this PR do?
Fixes mutable default arguments and unclosed file handles across several files.
Mutable defaults (can cause shared state across calls):
debug_utils.py:DebugUnderflowOverflow.__init__trace_batch_nums=[]->Nonekernel_config.py:KernelConfig.__init__kernel_mapping={}->Nonemodeling_idefics.py:freeze_model,freeze_text_layers,freeze_vision_layersmodule_exceptions=[]->()convert_usefulsensors_to_hf.py:_read_h5_weightsweights={}->None(this dict is actively mutated via assignment at line 50)Resource leaks (file handles not closed):
processing_utils.py:open(template_file).read()in dict comprehension -> explicit loop with context managertokenization_myt5.py:json.load(open(vocab_file))-> context managercli/serve.py:config_path.open().read()-> context managerWho can review?
Anyone in the community is free to review the PR.