Skip to content

Conversation

@artem1984A
Copy link

No description provided.

@artem1984A
Copy link
Author

Applied all suggestions:
Simplified test to use hardcoded expected values (7e3adae)

  • Removed comparison with original VarMap implementation
  • Tests now directly verify against known expected values
  • Makes tests clearer and more maintainable

Used std::env::temp_dir() for cross-platform compatibility (4bc438e)

  • Replaced hardcoded /tmp/ paths with OS-agnostic temporary directory
  • Now should works correctly on Windows, macOS, Linux, and other platforms

Removed manual memory leak in batch operations (5930869)

  • Changed from Box::leak() to Vec<String> + .as_str() pattern

@ivarflakstad
Copy link
Member

Apologies for the delay!

Now that I've taken a step back and had a second look at this, I'm actually a bit unsure if this has a real usecase.
For training we use the Mutex variant, which makes sense.
But for inference we shouldn't really use varmaps in the first place. You should use something like VarBuilder::from_mmaped_safetensors, or one of the other variants that have no locking at all.

@artem1984A
Copy link
Author

Thank You for the thorough review and feedback.
I'm just enthusiast in mathematics and AI models (officially a backend developer),
and I initially thought ConcurrentVarMap could help with multi-threaded inference
scenarios involving mutable weights.
However, after analyzing my own quantization experiments, I realize I'm still need only
direct SafeTensors access (memory-mapped, thread-safe), which is the correct approach
for this use cases.
I apologize if this contribution doesn't address a real need at this time.
If I will complete improvements to my works in quantization tools in the future, I'd be grateful
for Your review of those contributions.
Thank You once more for time and insights!

@ivarflakstad
Copy link
Member

Shame I didn't catch the issue sooner. Apologies for wasting your time. At least we both learned some valuable details 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants