Skip to content

Conversation

@Daraan
Copy link
Contributor

@Daraan Daraan commented Nov 2, 2025

Currently are deprecation warnings sometimes not informative enough. The the warning is triggered it does not tell us where the deprecated feature is used. For example, ray internally raises a deprecation warning when an RLModuleConfig is initialized.

>>> from ray.rllib.core.rl_module.rl_module import RLModuleConfig
>>> RLModuleConfig()
2025-11-02 18:21:27,318 WARNING deprecation.py:50 -- DeprecationWarning: `RLModule(config=[RLModuleConfig object])` has been deprecated. Use `RLModule(observation_space=.., action_space=.., inference_only=.., model_config=.., catalog_class=..)` instead. This will raise an error in the future!

This is confusing, where did I use a config, what am I doing wrong? This raises issues like: https://discuss.ray.io/t/warning-deprecation-py-50-deprecationwarning-rlmodule-config-rlmoduleconfig-object-has-been-deprecated-use-rlmodule-observation-space-action-space-inference-only-model-config-catalog-class-instead/23064

Tracing where the error actually happens is tedious - is it my code or internal? The output just shows deprecation.:50. Not helpful.

This PR adds a stacklevel option with stacklevel=2 as the default to all deprecation_warnings. So devs and users can better see where is the deprecated option actually used.

@Daraan Daraan requested review from edoakes and jjyao as code owners November 2, 2025 17:26
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a stacklevel parameter to the deprecation_warning function, which is a great improvement for debugging as it helps pinpoint the source of deprecation warnings. The implementation is correct, setting a sensible default of stacklevel=2 for direct calls and stacklevel=3 within the @Deprecated decorator to correctly point to user code.

While the change is functionally sound, it would be greatly strengthened by adding unit tests to verify that the stacklevel parameter works as expected and that warnings are reported from the correct location in the code. For example, you could assert the pathname and lineno on the LogRecord object passed to a mocked logger.

I have one minor suggestion to improve code style.

else:
logger.warning(
"DeprecationWarning: " + msg + " This will raise an error in the future!"
"DeprecationWarning: " + msg + " This will raise an error in the future!",
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better readability and consistency with modern Python practices, consider using an f-string for string formatting instead of concatenation.

Suggested change
"DeprecationWarning: " + msg + " This will raise an error in the future!",
f"DeprecationWarning: {msg} This will raise an error in the future!",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is how the original code is written, I would modernize it to "DeprecationWarning: %s This will raise an error in the future!" and use arguments. Depends on what you prefer.

@ray-gardener ray-gardener bot added rllib RLlib related issues train Ray Train Related Issue core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core rllib RLlib related issues train Ray Train Related Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant