Fix EventPublisherFactory logic for disabled KV cache events and publisher is "null"

[usberkeley]

Purpose

Fixed a logic flaw in the EventPublisherFactory.create() method to ensure it properly returns NullEventPublisher when enable_kv_cache_events=False or publisher="null".

Changes

• Fixed conditional logic in EventPublisherFactory.create() method
• When config.publisher == "null", directly return an instance of NullEventPublisher() to avoid calling the constructor.
  Otherwise, the code will attempt to retrieve the constructor of NullEventPublisher from the registry and call it with parameters. However, NullEventPublisher does not accept these parameters, resulting in a TypeError.

Test Plan

• New test cases verify correct behavior across various configuration combinations
• Ensures backward compatibility
• All existing tests continue to pass

Test Result

Pass

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request addresses a logic flaw in the EventPublisherFactory.create method. The changes correctly ensure that a NullEventPublisher is returned when KV cache events are disabled (enable_kv_cache_events=False) or when the publisher is explicitly set to "null". This prevents a TypeError that would occur when trying to instantiate NullEventPublisher with unexpected arguments. The fix is implemented cleanly by adjusting the initial conditional check. Furthermore, a new test suite has been added, which thoroughly validates the factory's behavior across various configurations, including disabled events, different publisher types, and default settings. The changes are correct, well-tested, and improve the robustness of the event publisher creation logic.

[markmc]

lgtm, thanks!

A suggestion for better (IMO) improvement inline

[markmc]

Just an observation, not a problem with this PR - if we ever add another publisher other than ZmqEventPublisher it will be required to accept all the members of KEventsConfig as constructor parameters, which doesn't make sense

[usberkeley]

Yep, new EventPublisher may not need to implement all parameters of KEventsConfig

[markmc]

Maybe this whole NullEventPublisher thing just isn't very useful ... we could just return None and make scheduler like this:

        self.kv_event_publisher = EventPublisherFactory.create(
            self.kv_events_config,
            self.parallel_config.data_parallel_rank,
        )

        self.kv_cache_manager = KVCacheManager(
            ...
            enable_kv_cache_events=self.kv_event_publisher is not None,
            ...
        )

       # publish collected KV cache events                                                                                                                                    
        if events:
            assert self.kv_event_publisher is not None

wdyt?

[usberkeley]

@markmc Wow, perfect, I completely agree with this revision direction. It has the following benefits:

1. Scheduler: Avoids redundant checks for whether KV Cache Event is enabled.

self.enable_kv_cache_events = (
    self.kv_events_config is not None
    and self.kv_events_config.enable_kv_cache_events
)

2. KVCacheManager: The parameter enable_kv_cache_events only stores events in the event queue when an EventPublisher exists, so assigning this parameter based on self.kv_event_publisher is not None is reasonable.

I understand that the original author intended to use the Null Object Design Pattern, returning a NullEventPublisher to avoid redundant null-check logic. However, using None directly seems simpler.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @usberkeley.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[mergify]

Documentation preview: https://vllm--27257.org.readthedocs.build/en/27257/

[hmellor]

Looks like a bad merge has occurred and everyone has been notified.

I think this issue should be already fixed by #26915.

Please make a PR if I am wrong.

[usberkeley]

Looks like a bad merge has occurred and everyone has been notified.

I think this issue should be already fixed by #26915.

Please make a PR if I am wrong.

hi @hmellor
I'm very sorry for the mistake made during the code rebase, which caused everyone to be notified.