Implement MonitoredChannel - #718
Conversation
|
/unit_test |
GiGL Automation@ 21:34:08UTC : 🔄 @ 21:35:52UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 21:34:10UTC : 🔄 @ 21:43:27UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 21:34:10UTC : 🔄 |
| # 32 - 39 | write_block_id_ | size_t | 8 bytes | Total messages enqueued <-- READ HERE | ||
| # 40 - 47 | read_block_id_ | size_t | 8 bytes | Total messages dequeued <-- READ HERE |
There was a problem hiding this comment.
These are monotonically increasing sequence numbers, not wraparound array indices. Hence we can compute queue size as write_block_id_ - read_block_id_
| logger.warning( | ||
| "initialize_metrics() was not called, using NopMetricsPulisher as default" | ||
| task_config_uri = os.environ.get(TASK_CONFIG_URI_ENV_KEY) | ||
| service_name = os.environ.get(JOB_NAME_GROUPING_ENV_KEY) or os.environ.get( |
There was a problem hiding this comment.
It's actually "GIGL_APPLIED_TASK_IDENTIFIER" this serves as the appropriate service name based on frozen config. Should double check if "GBML_JOB_NAME" is set and if so then this can be simplified
There was a problem hiding this comment.
Why don't we call initialize_metrics first? I'm. a little confused as to why we're making this change
| super().__init__(*args, **kwargs) | ||
| self._finalizer: Optional[weakref.finalize] = None | ||
|
|
||
| def qsize(self) -> int: |
There was a problem hiding this comment.
BTW would it be easier to just copy over / re-implement our own CPP share memory channel?
We support cpp now - though I think there may be some fiddling required to setup GLT imports in our CPP code.
But it seems like a better long term solution to just add a qsize to the shm cpp directly, WDYT?
There was a problem hiding this comment.
I agree that implementing our own CPP shared memory channel is the most robust long term solution. But this would be a rather large change, requiring a few different moving parts copied over. I suspect we are not too worried about upstream features for these queues on the GLT side, but worth considering too.
I think the options are something like:
- Copy everything over from GLT. Gives us full control but large change.
- Build GLT from source with minimal changes to support
qsize. Not worth the hassle in my opinion. - Try to extend GLT using our own bindings to support
qsize. This would be best but (a) queue meta is private field of the channel, and the fields we read to derive theqsizeare private within meta (friendship would not be inherited to our subclasses). - Use the shared memory inspection as we currently have.
If it's okay, we can keep the implementation as is, leave a TODO, and consider full channel re-implementation later if we think it's worth it.
| logger.warning( | ||
| "initialize_metrics() was not called, using NopMetricsPulisher as default" | ||
| task_config_uri = os.environ.get(TASK_CONFIG_URI_ENV_KEY) | ||
| service_name = os.environ.get(JOB_NAME_GROUPING_ENV_KEY) or os.environ.get( |
There was a problem hiding this comment.
Why don't we call initialize_metrics first? I'm. a little confused as to why we're making this change
Purpose
The purpose of this PR is to add telemetry to our
ShmChannel.We want to know how many
SampleMessage's are in each of our channels at any given time, since an empty channel implies a blockingrecv, which implies a dataloader bottleneck. Conversely, if our channels stay comfortably above some low watermark then any changes upstream ofdataloader.__next__is unlikely to improve throughput.Context (Colocated-Mode)
Each of our
Nreplicas launchN_localtraining workers. Each training worker initializes one or more data loaders (e.g.DistABLPLoader,DistNeighborLoader). Each dataloader is associated with aShmChannel. For a given dataloader, multiple sampling workers (e.g.DistNeighborSampler,DistPPRNeighborSampler) push data into the queue.The channel is roughly used like:
Challenge
There is no direct way to gauge the queue size since GLT's
ShmChanneldoes not expose the size to python.PR Changes
SizedShmChannel(ShmChannel): addsqsize()method toShmChannelby attaching to the channels memory region and inspecting the C++ struct layout. It calculates queue size as the delta between the circular buffer's write and read sequence numbers.MonitoredShmChannel(SizedShmChannel): a subclass that automatically integrates with the GiGL metrics service, pushing theqsizeas a gauge on everyrecv()call.MonitoredShmChannelinstead of aShmChannelinitialize_metrics()in the GiGL runtime setup but this doesn't propogate to the processes which actually own theMonitoredChannel. I've made it such thatget_metrics_service_instanceattempts to implicitly callinitialize_metrics(task_config_uri, service_name)if the relevant environment variables are set (which they are in the VAI workers). This enables us to integrate with anOpsMetricPublisherfrom the training code.Things to Verify
ShmChannelqueue size.ShmChannel.Updated Changelog.md? NO
Ready for code review?: YES