Skip to content

[fix] Resolve CI error due to import error#256

Merged
CharlieFRuan merged 2 commits intoNovaSky-AI:mainfrom
tyler-griggs:tgriggs/v_ci_fix
Sep 8, 2025
Merged

[fix] Resolve CI error due to import error#256
CharlieFRuan merged 2 commits intoNovaSky-AI:mainfrom
tyler-griggs:tgriggs/v_ci_fix

Conversation

@tyler-griggs
Copy link
Member

@tyler-griggs tyler-griggs commented Sep 8, 2025

We cannot import VerifiersGenerator unless --with verifiers is included in the uv command. An error is thrown when pytest is collecting all tests and tries to import VerifiersGenerator.

This PR moves the import of VerifiersGenerator in the test into the test body so that it's only imported when executed.

Confirmed that Verifiers integration tests still pass with the commands in the CI pipeline:

Screenshot 2025-09-07 at 10 48 53 PM

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 resolves a CI error that occurs during pytest collection by moving an import with an optional dependency into a test function. The change is effective and correctly solves the problem. I have provided one suggestion to use pytest.importorskip as a more idiomatic alternative for handling optional dependencies in tests, which could improve the maintainability of the code.

}
)

from integrations.verifiers.verifiers_generator import VerifiersGenerator
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

While this local import solves the ImportError during test collection, a more idiomatic pytest approach is to use pytest.importorskip() at the top of the test module. This would allow you to keep the VerifiersGenerator import at the top level, and it would clearly mark all tests in this file to be skipped if the verifiers package isn't installed. This is often cleaner and more maintainable for test files that depend on optional integrations.

For example, you could add the following at the top of the file and remove this local import:

import pytest

pytest.importorskip("verifiers")

from integrations.verifiers.verifiers_generator import VerifiersGenerator

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that might be true but I also don't want tests to be silently skipped

@tyler-griggs tyler-griggs changed the title [fix] Resolve CI error due to Environments Hub integration [fix] Resolve CI error due to import error Sep 8, 2025
@CharlieFRuan CharlieFRuan merged commit 29aeddc into NovaSky-AI:main Sep 8, 2025
3 checks passed
dzorlu referenced this pull request in fleet-ai/SkyRL Feb 4, 2026
We cannot import `VerifiersGenerator` unless `--with verifiers` is
included in the `uv` command. An error is thrown when pytest is
collecting all tests and tries to import `VerifiersGenerator`.

This PR moves the import of `VerifiersGenerator` in the test into the
test body so that it's only imported when executed.

Confirmed that Verifiers integration tests still pass with the commands
in the CI pipeline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants