Skip to content

[Train] Isolating Ray Train Circular Dependency Tests #57710

Merged
matthewdeng merged 3 commits intoray-project:masterfrom
JasonLi1909:isolate-circular-dependency-tests
Oct 17, 2025
Merged

[Train] Isolating Ray Train Circular Dependency Tests #57710
matthewdeng merged 3 commits intoray-project:masterfrom
JasonLi1909:isolate-circular-dependency-tests

Conversation

@JasonLi1909
Copy link
Contributor

@JasonLi1909 JasonLi1909 commented Oct 14, 2025

This PR updates Ray Train's circular dependency tests to isolate each import statement in the test within a separate subprocess. This is because each import has the side effect of mutating sys.modules which can cause other imports to pass when they could have failed in an isolated subprocess.

@JasonLi1909 JasonLi1909 requested a review from a team as a code owner October 14, 2025 20:16
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 PR introduces a robust way to test for circular dependencies by isolating each import in a separate subprocess. This is a great improvement to the test suite's reliability. My review focuses on further enhancing the test structure by using pytest's parameterization feature, which will make the tests more readable, maintainable, and provide more granular feedback on failures.

@ray-gardener ray-gardener bot added the train Ray Train Related Issue label Oct 15, 2025
@JasonLi1909 JasonLi1909 requested a review from justinvyu October 15, 2025 18:04
@JasonLi1909 JasonLi1909 added the go add ONLY when ready to merge, run all tests label Oct 15, 2025
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Nice!

cursor[bot]

This comment was marked as outdated.

Signed-off-by: JasonLi1909 <[email protected]>
@JasonLi1909 JasonLi1909 force-pushed the isolate-circular-dependency-tests branch from 05798d8 to 7cfe22d Compare October 16, 2025 21:56
cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

🤝

@matthewdeng matthewdeng merged commit f917fde into ray-project:master Oct 17, 2025
6 checks passed
justinyeh1995 pushed a commit to justinyeh1995/ray that referenced this pull request Oct 20, 2025
)

This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
xinyuangui2 pushed a commit to xinyuangui2/ray that referenced this pull request Oct 22, 2025
)

This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
Signed-off-by: xgui <[email protected]>
elliot-barn pushed a commit that referenced this pull request Oct 23, 2025
This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025
)

This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025
)

This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
Signed-off-by: Aydin Abiar <[email protected]>
Future-Outlier pushed a commit to Future-Outlier/ray that referenced this pull request Dec 7, 2025
)

This PR updates Ray Train's circular dependency tests to isolate each
import statement in the test within a separate subprocess. This is
because each import has the side effect of mutating `sys.modules` which
can cause other imports to pass when they could have failed in an
isolated subprocess.

---------

Signed-off-by: JasonLi1909 <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests train Ray Train Related Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants