patches on 1.70 - #356
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
Changes are minimal, consistent with existing versioning (v1.70.00) and should improve CI reliability without altering runtime logic.
Pull request overview
Aligns the client-reported patch version with the current TransferBench release (v1.70.00) and adjusts the CodeQL CI setup to install git more reliably in the container environment.
Changes:
- Updates
CLIENT_VERSIONto"00"so the client reportsv1.70.00(matching the changelog/versioning). - Simplifies CodeQL workflow OS setup by removing the git PPA steps and installing
gitviaapt-getwith noninteractive settings.
File summaries
| File | Description |
|---|---|
src/client/EnvVars.hpp |
Sets client patch version to 00 to match TransferBench::VERSION/release versioning (1.70.00). |
.github/workflows/codeql.yml |
Updates apt install steps to install git noninteractively and removes PPA-related commands. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Pingpong post-prep currently continues when a pong-half resource is missing, which can leave a null localFlagMem and later cause a hang/crash in the pingpong GPU kernel.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| auto it = pongByTransferIdx.find(pingRss->transferIdx); | ||
| if (it == pongByTransferIdx.end()) continue; | ||
| TransferResources* pongRss = it->second; |
gilbertlee-amd
left a comment
There was a problem hiding this comment.
Remove ping pong latency test
There was a problem hiding this comment.
🟡 Changes recommended
The new pingpong GPU kernel uses 1-byte atomic store builtins even under NVCC despite CUDA lacking 1-byte atomics, risking incorrect synchronization or hangs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| // TODO: replace with hip_atomic_store | ||
| if (!useSrcMem) { | ||
| if (isPing) { | ||
| __atomic_store_n((uint8_t*)remoteFlag, val, __ATOMIC_RELEASE); | ||
| GpuWait(localFlag, val); | ||
| } else { | ||
| GpuWait(localFlag, val); | ||
| __atomic_store_n((uint8_t*)remoteFlag, val, __ATOMIC_RELEASE); | ||
| } | ||
| } else{ | ||
| uint8_t* const srcPtr = val ? srcVal1 : srcVal0; | ||
| if (isPing) { | ||
| __atomic_store((uint8_t*)remoteFlag, srcPtr, __ATOMIC_RELEASE); | ||
| GpuWait(localFlag, val); | ||
| } else { | ||
| GpuWait(localFlag, val); | ||
| __atomic_store((uint8_t*)remoteFlag, srcPtr, __ATOMIC_RELEASE); | ||
| } | ||
|
|
||
| } |
Motivation
Missed minor version in client in previous merge.
Also updating CI install requirements to fix failed CI test.