Skip to content

Validate remote read byte counts#981

Open
fallintoplace wants to merge 2 commits into
rapidsai:mainfrom
fallintoplace:fix-remote-read-short-transfers
Open

Validate remote read byte counts#981
fallintoplace wants to merge 2 commits into
rapidsai:mainfrom
fallintoplace:fix-remote-read-short-transfers

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • validate successful easy-curl transfers against the callback byte count
  • reset callback byte accounting before each easy-curl retry attempt
  • reset the existing device bounce state before retrying device reads
  • fail easy and multi-poll remote reads when the callback receives fewer bytes than requested

Why

A remote read could return success after libcurl completed even if the write callback received fewer bytes than requested. The easy retry path also reused the same callback offset between attempts, so a partial timed-out attempt could make the next attempt append at the wrong offset.

This keeps the existing host and device callback paths, restarts per-attempt byte state on retries, and rejects successful transfers that did not deliver the requested byte count.

Validation

  • pre-commit run --files cpp/include/kvikio/detail/remote_callback.hpp cpp/src/detail/remote_callback.cpp cpp/include/kvikio/shim/libcurl.hpp cpp/src/shim/libcurl.cpp cpp/src/remote_handle.cpp cpp/src/detail/multi_poll_reactor.cpp python/kvikio/tests/test_http_io.py
  • python3 -m py_compile python/kvikio/tests/test_http_io.py
  • git diff --check

Not run locally: the KvikIO Python/C++ test suite, because this checkout does not have an importable kvikio package and CUDA tooling/GPU access are unavailable (nvcc and nvidia-smi are not present).

@fallintoplace fallintoplace requested review from a team as code owners June 15, 2026 22:09
@copy-pr-bot

copy-pr-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@madsbk madsbk added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 16, 2026
@madsbk

madsbk commented Jun 16, 2026

Copy link
Copy Markdown
Member

/ok to test 164a360

Comment thread cpp/src/remote_handle.cpp Outdated
KVIKIO_CUDA_DRIVER_TRY(cudaAPI::instance().StreamSynchronize(stream));
copied += chunk_size;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to needlessly introduce another level of intermediate bounce buffer. Why is it not possible to just use the existing setup and check the relevant error condition as part of curl.perform?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your attention, I have amended.

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

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants