[Bug] Fix memory profiler number formatting - #818
Conversation
|
Hi, I notice this is marked 'draft'. Is that intentional? If so, I'll wait till it is in 'ready for review' before proceeding further. |
|
(notably, I cannot run |
|
To use Codex here, create a Codex account and connect to github. |
|
My apologies, @hughperkins ! I've marked this and #817 ready for review now |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b06ddc71c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fb.finalize() | ||
| x[0] = 1 | ||
|
|
||
| qd.profiler.print_memory_profiler_info() |
There was a problem hiding this comment.
On systems without the exact en_US.UTF-8 locale—such as the provided Linux environment, whose locale -a only lists C, C.utf8, and POSIX—this call throws from std::locale("en_US.UTF-8") in LlvmRuntimeExecutor::print_memory_profiler_info() before reaching the corrected format strings. Because the new CPU test has no locale guard or fallback, it introduces a platform-dependent test failure and leaves the profiler unusable in those installations; the implementation should format with a locally available/fallback locale rather than assuming this named locale exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Opus concurs with codex:
Please could you take a look? (Or I can get Opus to address this, if you prefer?)
|
Asked opus about this PR. It says the PR looks good, and fixes an actual hard crash. 
|
|
Please could you check the codex comment above? |
Replace the {:L}/global-locale approach in the memory profiler with
fmt::group_digits(...), which inserts thousand separators
locale-independently. This removes the process-wide
std::locale::global(std::locale("en_US.UTF-8")) mutation and its hard
dependency on the en_US.UTF-8 locale being installed, which threw
std::runtime_error (crashing the profiler and the new CPU test) on hosts
that only ship C / C.UTF-8 / POSIX locales.
Pack the explanatory comment lines nearer the project's 120-char width instead of leaving them short.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Once agent has removed the superfluous comment, I think I'll wait for CI to ~green, then merge |
fmt::group_digits(...) is self-documenting; the explanatory comment adds no information.
hughperkins
added
awaiting-ci-to-pass
|
Thanks! |
2 participants
Updates the memory profiler's locale-aware integer formatting for bundled fmt 10 and exercises the sparse-tree output paths in a CPU regression test.
Originally reported in taichi-dev/taichi#8663.
This fixes profiler output only; it does not change allocator lifetimes.
Checks: editable build; memory-profiler test (1 passed); pre-commit.