Skip to content

Conversation

@raulchen
Copy link
Contributor

@raulchen raulchen commented Jan 7, 2026

Summary

Separates backend-specific tests from engine-level tests, preparing for multiple backend implementations.

Changes

  • Removed 7 tests that are specific to JaxBackend from test_engine.py to test_jax_backend.py
  • Kept 2 tests that test engine-specific logic
  • engine.py: extracted prepare_model_pass_batch() and prepare_sample_batch() to module level for test imports

Test coverage remains the same as before.
Note, there are still some functions not covered by current tests. But that's out of this PR's scope.

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 effectively separates backend-specific tests from engine-level tests, which is a great step towards supporting multiple backend implementations. The changes are well-executed: 7 Jax-specific tests have been moved from test_engine.py to test_jax_backend.py, and the remaining tests in test_engine.py are indeed engine-specific. The refactoring in engine.py to extract prepare_model_pass_batch() and prepare_sample_batch() into module-level functions is a clean way to enable their use in the new backend tests. The moved tests have been correctly adapted to interact with the JaxBackend directly. Overall, this is a solid refactoring that improves the test structure and maintainability. The code quality is high, and I have no further recommendations for changes.

@pcmoritz pcmoritz added the tx label Jan 8, 2026
# Conflicts:
#	skyrl-tx/tests/tinker/test_engine.py
#	skyrl-tx/tests/tinker/test_jax_backend.py
@pcmoritz
Copy link
Collaborator

pcmoritz commented Jan 8, 2026

This fixes #819

@pcmoritz pcmoritz closed this Jan 8, 2026
@pcmoritz pcmoritz reopened this Jan 8, 2026
@pcmoritz pcmoritz merged commit 3fcbc05 into NovaSky-AI:main Jan 8, 2026
6 checks passed
@raulchen raulchen deleted the refactor-test-engine2 branch January 8, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants