[BugFix] dummy import some ops #5192
Conversation
|
Thanks for your contribution! |
There was a problem hiding this comment.
Pull request overview
This PR adds a try-except wrapper around the platform-specific imports and function definitions in fastdeploy/cache_manager/ops.py. The purpose appears to be handling import errors gracefully by setting all functions to None when imports fail, though the PR title and description lack detail about the motivation.
- Added Apache 2.0 license header to the file
- Wrapped existing platform-specific import logic in a try-except block
- Added fallback that sets all functions to None if imports fail
Comments suppressed due to low confidence (1)
fastdeploy/cache_manager/ops.py:90
- Except block directly handles BaseException.
except:
| cuda_host_alloc = None | ||
| cuda_host_free = None | ||
| set_data_ipc = None | ||
| share_external_data_ = None | ||
| swap_cache_all_layers = None | ||
| unset_data_ipc = None | ||
| set_device = None | ||
| memory_allocated = None | ||
| get_output_kv_signal = None | ||
| get_data_ptr_ipc = None | ||
| ipc_sent_key_value_cache_by_remote_ptr = None | ||
| ipc_sent_key_value_cache_by_remote_ptr_block_sync = None | ||
| get_peer_mem_addr = None | ||
| get_all_visible_devices = None |
There was a problem hiding this comment.
Silent failure with no logging: The except block silently sets all functions to None without any logging or warning. This makes it extremely difficult to diagnose issues when imports fail.
Consider adding logging to help with debugging:
except (ImportError, ModuleNotFoundError) as e:
import logging
logger = logging.getLogger(__name__)
logger.warning(f"Failed to import platform-specific ops: {e}. Setting fallback values.")
# Set None valuesThis would help developers and users understand why operations are unavailable.
| cuda_host_alloc = None | ||
| cuda_host_free = None | ||
| set_data_ipc = None | ||
| share_external_data_ = None | ||
| swap_cache_all_layers = None | ||
| unset_data_ipc = None | ||
| set_device = None | ||
| memory_allocated = None | ||
| get_output_kv_signal = None | ||
| get_data_ptr_ipc = None | ||
| ipc_sent_key_value_cache_by_remote_ptr = None | ||
| ipc_sent_key_value_cache_by_remote_ptr_block_sync = None | ||
| get_peer_mem_addr = None | ||
| get_all_visible_devices = None | ||
|
|
There was a problem hiding this comment.
Setting functions to None can cause AttributeError at runtime: When these functions are set to None in the except block, any code that tries to call them (e.g., cuda_host_alloc()) will fail with TypeError: 'NoneType' object is not callable. This defers error detection to runtime rather than providing clear feedback at import time.
Consider either:
- Raising a clear error at import time if these operations are critical
- Creating stub functions that raise informative errors when called:
def _create_stub(name):
def stub(*args, **kwargs):
raise RuntimeError(f"{name} is not available: platform-specific ops failed to import")
return stub
# Then in the except block:
cuda_host_alloc = _create_stub("cuda_host_alloc")
cuda_host_free = _create_stub("cuda_host_free")
# etc.This provides better error messages when the functions are actually used.
| cuda_host_alloc = None | |
| cuda_host_free = None | |
| set_data_ipc = None | |
| share_external_data_ = None | |
| swap_cache_all_layers = None | |
| unset_data_ipc = None | |
| set_device = None | |
| memory_allocated = None | |
| get_output_kv_signal = None | |
| get_data_ptr_ipc = None | |
| ipc_sent_key_value_cache_by_remote_ptr = None | |
| ipc_sent_key_value_cache_by_remote_ptr_block_sync = None | |
| get_peer_mem_addr = None | |
| get_all_visible_devices = None | |
| def _create_stub(name): | |
| def stub(*args, **kwargs): | |
| raise RuntimeError(f"{name} is not available: platform-specific ops failed to import") | |
| return stub | |
| cuda_host_alloc = _create_stub("cuda_host_alloc") | |
| cuda_host_free = _create_stub("cuda_host_free") | |
| set_data_ipc = _create_stub("set_data_ipc") | |
| share_external_data_ = _create_stub("share_external_data_") | |
| swap_cache_all_layers = _create_stub("swap_cache_all_layers") | |
| unset_data_ipc = _create_stub("unset_data_ipc") | |
| set_device = _create_stub("set_device") | |
| memory_allocated = _create_stub("memory_allocated") | |
| get_output_kv_signal = _create_stub("get_output_kv_signal") | |
| get_data_ptr_ipc = _create_stub("get_data_ptr_ipc") | |
| ipc_sent_key_value_cache_by_remote_ptr = _create_stub("ipc_sent_key_value_cache_by_remote_ptr") | |
| ipc_sent_key_value_cache_by_remote_ptr_block_sync = _create_stub("ipc_sent_key_value_cache_by_remote_ptr_block_sync") | |
| get_peer_mem_addr = _create_stub("get_peer_mem_addr") | |
| get_all_visible_devices = _create_stub("get_all_visible_devices") |
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | ||
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | ||
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") |
There was a problem hiding this comment.
Spelling error: "UNIMPLENENTED" should be "UNIMPLEMENTED".
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLEMENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") |
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | ||
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | ||
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") |
There was a problem hiding this comment.
Spelling error: "UNIMPLENENTED" should be "UNIMPLEMENTED".
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLEMENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") |
| @@ -1,77 +1,107 @@ | |||
| """ | |||
| # Copyright (c) 2025 PaddlePaddle Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
Extra space in copyright header: There are two spaces between "2025" and "PaddlePaddle" instead of one.
| # Copyright (c) 2025 PaddlePaddle Authors. All Rights Reserved. | |
| # Copyright (c) 2025 PaddlePaddle Authors. All Rights Reserved. |
| """ | ||
| # Copyright (c) 2025 PaddlePaddle Authors. All Rights Reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License" |
There was a problem hiding this comment.
Missing semicolon in license header: The Apache License header comment should end with ");" not just ")".
| # Licensed under the Apache License, Version 2.0 (the "License" | |
| # Licensed under the Apache License, Version 2.0 (the "License"); |
| else: | ||
| return "CUDA_VISIBLE_DEVICES=0,1,2,3,4,5,6,7" | ||
|
|
||
| except: |
There was a problem hiding this comment.
Bare except clause is a bad practice: Using a bare except: catches all exceptions including SystemExit, KeyboardInterrupt, and other exceptions that should typically propagate. This can mask unexpected errors and make debugging difficult.
Consider catching specific exceptions instead:
except (ImportError, ModuleNotFoundError, AttributeError) as e:
# Log the error for debugging
# Then set fallback valuesThis would catch import-related errors while allowing critical exceptions to propagate properly.
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | ||
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | ||
|
|
||
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | ||
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") |
There was a problem hiding this comment.
Spelling error: "UNIMPLENENTED" should be "UNIMPLEMENTED".
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLENENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLENENTED") | |
| raise RuntimeError("XPU get_data_ptr_ipc UNIMPLEMENTED!") | |
| def ipc_sent_key_value_cache_by_remote_ptr(*args, **kwargs): | |
| raise RuntimeError("XPU ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") | |
| def ipc_sent_key_value_cache_by_remote_ptr_block_sync(*args, **kwargs): | |
| raise RuntimeError("XPU No ipc_sent_key_value_cache_by_remote_ptr UNIMPLEMENTED") |
| @@ -1,77 +1,107 @@ | |||
| """ | |||
There was a problem hiding this comment.
PR Title Missing Required Tag: The PR title "dummy import fd" does not follow the required format of [CLASS]Title. According to the coding guidelines and checklist, it should include a tag such as [BugFix], [Feature], [Optimization], etc. For example: "[BugFix] Add fallback handling for import errors" or "[Feature] Add error handling for platform-specific imports".
| @@ -1,77 +1,107 @@ | |||
| """ | |||
There was a problem hiding this comment.
PR Description Incomplete: The PR description does not explain the motivation or modifications being made. According to the coding guidelines, the description should at minimum explain why these modifications are being made and what problem is being solved. Please provide:
- Motivation: Why is the try-except block being added?
- Modifications: What specific issue does wrapping the imports in try-except solve?
- Usage/Command: How should this change be used or tested?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5192 +/- ##
==========================================
Coverage ? 58.04%
==========================================
Files ? 317
Lines ? 38472
Branches ? 5765
==========================================
Hits ? 22332
Misses ? 14345
Partials ? 1795
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Motivation
存在不编译FD而import fd源码的使用方式，需要去掉ops的import
Modifications
try except一下
Usage or Command
Accuracy Tests
Checklist
[FDConfig],[APIServer],[Engine],[Scheduler],[PD Disaggregation],[Executor],[Graph Optimization],[Speculative Decoding],[RL],[Models],[Quantization],[Loader],[OP],[KVCache],[DataProcessor],[BugFix],[Docs],[CI],[Optimization],[Feature],[Benchmark],[Others],[XPU],[HPU],[GCU],[DCU],[Iluvatar],[Metax]]pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.