RHOAIENG-26843, RHOAIENG-26066: tests: add scikit-learn smoke test #1171
Conversation
Adds a smoke test to verify basic scikit-learn functionality in JupyterLab images. This test trains a simple Logistic Regression model and checks predictions to ensure the scikit-learn installation is working correctly after package updates.
WalkthroughA new test method was added to the JupyterLab container image test suite that runs a Python script inside the container to train and predict with a logistic regression model using scikit-learn. The test verifies the output and ensures the container lifecycle is handled properly. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
jiridanek
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jiridanek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/containers/workbenches/jupyterlab/jupyterlab_test.py (1)
118-118: Fix inline comment formatting.Static analysis detected spacing issues with inline comments.
- @allure.issue("RHOAIENG-27131") # Assuming a new issue number for this test + @allure.issue("RHOAIENG-27131") # Assuming a new issue number for this test- print(f"Script output:\n{output_str}") # For debugging + print(f"Script output:\n{output_str}") # For debuggingAlso applies to: 163-163
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/containers/workbenches/jupyterlab/jupyterlab_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/containers/workbenches/jupyterlab/jupyterlab_test.py
118-118: Insert at least two spaces before an inline comment
Insert spaces
(E261)
163-163: Insert at least two spaces before an inline comment
Insert spaces
(E261)
🔇 Additional comments (3)
tests/containers/workbenches/jupyterlab/jupyterlab_test.py (3)
122-143: Well-designed smoke test script.The scikit-learn test script is well-structured with appropriate assertions and covers the essential functionality: importing libraries, training a model, making predictions, and verifying results.
120-170: Excellent implementation following established patterns.The test method follows the same robust pattern as other tests in the class:
- Proper container lifecycle management with try/finally
- Appropriate use of temporary directories for file handling
- Comprehensive output verification with both exit code and content checks
- Clean integration with the existing test infrastructure
The smoke test will effectively detect regressions in scikit-learn functionality as intended.
118-118: Verify the Allure issue number.The comment indicates this is an assumed issue number. Ensure the correct issue number is used before merging.
|
So, the ruff is unhappy, but otherwise this LGTM. |
|
I have the fix locally; looking at the code, imo it's very wrong... will have to make manual changes |
|
eh, never mind, it's not as bad as I thought (it must've improved copyting the script when I was asking for changes...) ok... still want to work on it a bit myself |
jiridanek
added
the
tide/merge-method-squash
…ink and minor script improvements
|
currently fails on some images because not everything contains sklearn |
Description
Adds a smoke test to
tests/containers/workbenches/jupyterlab/jupyterlab_test.pyto verify basic scikit-learn functionality. This test is intended to catch regressions in scikit-learn that might occur due to package updates, such as the one in #1126.The test:
sklearnandnumpy.LogisticRegressionmodel on a small sample dataset.How Has This Been Tested?
The new test
test_sklearn_smokewas added to the existing pytest suite. Due to sandbox limitations preventing access to the Docker daemon, the test could not be fully run to completion in the development environment. However, the test logic is standard and relies on the existingWorkbenchContainerinfrastructure. It is expected to pass in the CI environment where Docker is available.The following dependencies were installed to attempt local execution:
pytest<8,pytest-subtests,testcontainers,docker,pandas-stubs,types-requests,types-setuptools,allure-pytest,python-on-whales,podman.Merge criteria:
Summary by CodeRabbit