Update Python version requirements and enhance local CI testing - #32
Merged
Merged
Conversation
- Update README badge from Python 3.9+ to 3.10+ - Update TESTING.md to reflect Python 3.10, 3.11, 3.12 support - Update copilot instructions with new minimum version - Update type hints guidance for Python 3.10+ features
Add comprehensive local testing environment that replicates GitHub Actions CI: - environment-ci.yml: Conda environment matching CI dependencies - setup-ci-env.sh: Automated environment setup script - run-ci-tests.sh: Execute full CI test suite locally (lint, test, coverage) - test-all-python-versions.sh: Test across Python 3.10, 3.11, 3.12 - Makefile: Convenient commands (make setup-ci, make test, etc.) - quick-reference.sh: Quick reference card for common operations - CI-TESTING.md: Complete documentation Benefits: - Test locally before pushing to CI - Faster development iteration - Exact CI environment replication - Easy multi-version Python testing Tested successfully with Python 3.10: - All 205 unit tests passed - All 11 integration tests passed - Coverage: 48% overall
…performance_across_splits to include_groups=False
… in wishart_runs.py and wishart_ws.py
…t example scripts - Updated temperature calculation in `wishart_runs.py` to use `np.max` for better performance and clarity. - Added default value for `PBS_ARRAY_INDEX` in `wishart_runs.py` to prevent potential errors. - Suppressed specific warnings in `wishart_ws.py` related to deprecated packages. - Changed `metric_args` in `wishart_ws.py` to use `defaultdict` for better handling of metric arguments. - Enhanced error handling in `process_rerun` function to log exceptions in `wishart_ws.py`. - Updated type hints in `bootstrap.py` and `stochastic_benchmark.py` for better code clarity and type safety. - Added print statements in `stochastic_benchmark.py` to provide feedback during data loading and processing steps.
…etric_args handling
…mprove metric_args initialization
- Remove interpolated_results.pkl (839 MB) from tracking - Add patterns to exclude all checkpoint pkl files from version control - Generated checkpoint files should not be committed
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the project by updating the minimum Python requirement from 3.9 to 3.10 and introduces comprehensive local CI testing infrastructure. The changes improve type safety, developer workflow, and code portability.
Key Changes:
- Update minimum Python version to 3.10+ across all configuration and documentation files
- Add complete local CI testing infrastructure (scripts, environments, documentation)
- Fix type annotations and replace
type()checks withisinstance() - Improve path handling in examples to use relative paths for portability
Reviewed Changes
Copilot reviewed 19 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_bootstrap.py | Update test assertion to expect empty dict instead of None from defaultdict |
| src/bootstrap.py | Fix type annotations, improve error handling, add type guards |
| src/stochastic_benchmark.py | Add type hints, replace type() with isinstance(), add null checks |
| src/cross_validation.py | Add include_groups=False to pandas groupby.apply() calls |
| test-all-python-versions.sh | New script to test across Python 3.10, 3.11, 3.12 |
| setup-ci-env.sh | New script to create conda environments matching CI |
| run-ci-tests.sh | New script to run tests exactly as CI does |
| quick-reference.sh | New quick reference card for CI testing commands |
| environment-ci.yml | New conda environment matching CI dependencies |
| Makefile | New makefile with common development tasks |
| CI-TESTING.md | New comprehensive CI testing documentation |
| examples/wishart_n_50_alpha_0.5/wishart_ws.py | Convert hardcoded paths to relative paths, improve error handling |
| examples/wishart_n_50_alpha_0.5/wishart_runs.py | Convert hardcoded paths to relative paths, add default for env var |
| examples/wishart_n_50_alpha_0.5/README.md | New documentation for running the example |
| requirements-examples.txt | New optional dependencies file for examples |
| TESTING.md | Update Python version from 3.8/3.9 to 3.10+ |
| README.md | Update Python version badge and add example dependencies section |
| .github/copilot-setup-steps.yml | Update Python version references from 3.9+ to 3.10+ |
| .github/copilot-instructions.md | Update Python version compatibility notes |
As identified in code review, the parent attribute check in Experiment.evaluate_monotone() was unnecessary because: - The base Experiment class is never instantiated directly - All subclasses (ProjectionExperiment, RandomSearchExperiment, SequentialSearchExperiment) properly set self.parent in their __init__ methods - The parent is always a stochastic_benchmark instance, never None in practice Changes: - Removed __init__ method from Experiment base class - Removed the AttributeError check for parent being None - Added TYPE_CHECKING annotation to document that parent is set by subclasses - Updated docstring to reflect that parent is a stochastic_benchmark instance All 194 unit tests pass with these changes.
This commit fixes critical bugs identified in code review:
1. **wishart_ws.py**: Fixed update_rules() function signature
- Was: def update_rules(df) - missing self parameter
- Now: def update_rules(self, df) - correct signature
- Updated to use self.shared_args and self.metric_args instead of closure
2. **bootstrap.py**: Fixed two related bugs:
a) Corrected type annotation for update_rule
- Was: Callable[[pd.DataFrame], None]
- Now: Callable[['BootstrapParameters', pd.DataFrame], None]
- Reflects actual call signature: update_rule(bs_params, df)
b) Fixed default_update assignment to use unbound method
- Was: self.update_rule = self.default_update (bound method)
- Now: self.update_rule = BootstrapParameters.default_update (unbound)
- Prevents TypeError when called as update_rule(bs_params, df)
Root Cause:
- bootstrap.py calls update_rule as: bs_params.update_rule(bs_params, df)
- This requires update_rule to be an unbound function taking (self, df)
- The old code assigned a bound method, causing it to receive (self, bs_params, df)
- This would fail with 'takes 2 positional arguments but 3 were given'
Testing:
- All 25 bootstrap tests pass
- Verified default_update works when called through update_rule
- Verified custom update_rules functions work correctly
Credit: Bug identified in code review
These tests verify the critical update_rule functionality that was previously not properly tested: 1. test_update_rule_called_via_initBootstrap: - Verifies update_rule is called with correct signature (self, df) - Tests actual code path through initBootstrap() - Confirms parameters are passed correctly - Uses call tracking to verify execution 2. test_default_update_called_via_initBootstrap: - Tests the default_update assignment as unbound method - Verifies it works when called through update_rule attribute - Confirms default_update executes correctly with actual data - Would have caught the method binding bug 3. test_update_rule_signature_with_self_and_df: - Tests the wishart_ws.py usage pattern - Verifies custom update_rule modifies shared_args and metric_args - Confirms the (self, df) signature pattern works correctly - Demonstrates proper usage for documentation These tests prevent regressions of the bugs fixed in previous commits: - Missing self parameter in update_rule functions - Incorrect type annotation for update_rule - Bound method assignment instead of unbound Coverage: All 28 bootstrap tests pass (25 existing + 3 new)
This commit fixes a mathematical error introduced in commit 00f1ad2 where the refactoring incorrectly changed the calculation from column sums to row sums. ORIGINAL CODE: max(sum(abs(i) for i in qubo.A)) - Iterates over rows i in qubo.A - sum(abs(i) for i in qubo.A) performs element-wise addition of absolute values - Results in COLUMN sums (summing down each column) - Takes max of these column sums WRONG REFACTORING (commit 00f1ad2): np.max(np.abs(qubo.A).sum(axis=1)) - axis=1 computes ROW sums (sum across each row) - Produces different mathematical result CORRECT REFACTORING (this commit): np.max(np.abs(qubo.A).sum(axis=0)) - axis=0 computes COLUMN sums (sum down each column) - Mathematically equivalent to original EXAMPLE: A = [[1, -2, 3], [-4, 5, -6]] Original: max([5, 7, 9]) = 9 (column sums) Wrong: max([6, 15]) = 15 (row sums) ❌ Correct: max([5, 7, 9]) = 9 (column sums) ✅ This affects temperature calculations in the simulated annealing algorithm, which could significantly impact solution quality. Fixes: 00f1ad2 (Refactor temperature calculation...) Credit: Bug identified in code review
Owner
Author
|
@anurag-r20 @AzAINN @jvpcms check out this PR, I would like to merge it for everyone's reference |
This commit addresses reviewer feedback about the update_rule signature and documentation, improving clarity and consistency. Changes to src/bootstrap.py: - Enhanced docstring for update_rule parameter with detailed explanation - Added example showing correct usage pattern: def update_rule(self, df) - Clarified that it's an unbound method pattern - Updated type annotation description to mention both parameters - Improved default_update method documentation Changes to tests/test_bootstrap.py: - Standardized all dummy_update_rule functions to use 'self' parameter - Changed from: def dummy_update_rule(bs_params, df) - Changed to: def dummy_update_rule(self, df) - Added documentation explaining the unbound method pattern - Makes tests consistent with actual usage in examples (wishart_ws.py) Rationale: The previous inconsistency was confusing: - Type annotation: Callable[[BootstrapParameters, pd.DataFrame], None] - Old test code: def update_rule(bs_params, df) - Example code: def update_rule(self, df) - Documentation: didn't clearly explain the pattern The 'self' parameter name better communicates that this is an unbound method that will receive the BootstrapParameters instance as its first argument when called as: params.update_rule(params, df) All 28 bootstrap tests pass with these changes. Credit: Inconsistency identified in code review
These tests validate update_rule signatures and help prevent common mistakes
that would only be caught at runtime (or by careful code review).
New tests added:
1. test_update_rule_wrong_signature_fails:
- Tests that wrong signature (only df parameter) fails at runtime
- Validates TypeError is raised with clear message
- Uses inspect to verify signature is actually wrong
- Documents the failure mode for future developers
2. test_update_rule_signature_validation:
- Demonstrates signature validation helper pattern
- Tests multiple wrong signature variations:
* Missing self parameter: def update(df)
* Too many parameters: def update(self, df, extra)
* No parameters: def update()
- Shows how to validate before using update_rule
- Could be used for better error messages in production code
Value of these tests:
✅ Catch signature mistakes early in development
✅ Document expected signature clearly
✅ Prevent runtime failures in production
✅ Help onboard new contributors
✅ Serve as executable documentation
✅ Complement static type checking (Pylance catches at edit time)
The tests intentionally use wrong signatures to verify they fail correctly,
with type: ignore comments to suppress static analysis warnings.
Coverage: All 30 bootstrap tests pass (28 existing + 2 new)
Fixed 4 Pylance type errors caused by using defaultdict(lambda: None)
instead of defaultdict(dict).
Issues fixed:
- test_default_update_minimization (lines 130-136)
- test_default_update_maximization (lines 166-172)
Problem:
metric_args = defaultdict(lambda: None) # Returns None for missing keys
metric_args['RTT'] = {} # ❌ Type error: can't assign dict to None
Solution:
metric_args = defaultdict(dict) # Returns {} for missing keys
metric_args['RTT'] = {} # ✅ Type correct: dict to dict
The lambda: None pattern was creating a defaultdict that returns None
for missing keys, which conflicts with the type annotation expecting
DefaultDict[str, dict]. Using defaultdict(dict) is the correct pattern
for this use case.
All 30 bootstrap tests still pass.
- Remove commented populate_bs_results method (50+ lines) from stochastic_benchmark.py - Remove commented else clause that would have called populate_bs_results - Remove commented debugging code from sequential_exploration.py These code blocks were commented out during a Nov 2022 refactoring that split initialization into __init__ and initAll. Bootstrap results are now explicitly run via run_Bootstrap() method instead of auto-population. The commented code has not been used for 2+ years and can be safely removed. Addresses reviewer feedback on PR #32 about commented-out code indicating incomplete refactoring.
Instead of removing commented code entirely, replace it with clear documentation that: - Explains WHY the code pattern was changed (explicit control vs auto-population) - Points to git history for reference implementations - Guides future developers to the current design pattern - Preserves institutional knowledge without code clutter Changes: - stochastic_benchmark.py: Document that bootstrap is explicitly managed via run_Bootstrap(), not auto-populated (design from commit 857574f, Nov 2022) - sequential_exploration.py: Document that applyParallel replaced manual iteration for better performance This addresses reviewer feedback about commented-out code while preserving the value of showing alternative approaches for future development. Reverts and improves upon commit 304326a.
While np.unique() returns an ndarray that works in most contexts (iteration, etc.), the explicit list() conversions are kept for two reasons: 1. Type safety: RandomSearchParameters.budgets is typed as 'list', and Pylance correctly flags ndarray as incompatible. The conversion ensures type correctness. 2. Clarity: Explicit list type makes the intent clear and ensures compatibility with any downstream operations that specifically expect lists. Added explanatory comments to document this design decision and address reviewer feedback about seemingly unnecessary conversions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32 +/- ##
=======================================
Coverage ? 48.68%
=======================================
Files ? 14
Lines ? 2280
Branches ? 0
=======================================
Hits ? 1110
Misses ? 1170
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The initialization of upper_f = None and subsequent None check were unnecessary because the if-elif-else chain guarantees that upper_f is always assigned to a function before use: - If df is DataFrame or str → upper_f = upper_f_dataframe - If df is list of str → upper_f = upper_f_str_list - If df is list of DataFrame → upper_f = upper_f_df_list - Otherwise → raises TypeError (execution doesn't continue) The final 'if upper_f is None' check at line 477-478 could never be True, and the initialization only existed to silence a 'possibly unbound' warning. Simplifies code logic without changing behavior. Addresses reviewer feedback about unnecessary complexity.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…st behavior The name_fcn parameter is required for all code paths in Bootstrap_reduce_mem, but the validation was inside inner functions that are called later. This meant: - Validation happened AFTER name_fcn was already used (line 347, 390, 433) - Error would occur late in execution, not at function entry - Confusing error location for users Changes: 1. Added name_fcn validation at the start of Bootstrap_reduce_mem 2. Removed redundant checks from three inner functions: - upper_f_dataframe - upper_f_str_list - upper_f_df_list 3. Added test to verify fail-fast behavior Benefits: - Fails immediately with clear error message - Better user experience (error at function call, not deep in execution) - Cleaner code (single validation point instead of three) - Prevents confusing AttributeError from calling None Addresses reviewer feedback about validation check placement.
The docstring for update_rule was confusing because:
1. It used 'self' in the signature example, suggesting a bound method
2. It didn't clearly explain the unbound function pattern
3. The actual call is bs_params.update_rule(bs_params, df), not as a method
Changes to src/bootstrap.py:
1. Clarified that update_rule is an UNBOUND function pattern
2. Updated signature example to use 'bs_params' instead of 'self'
3. Explicitly showed how the function is called: update_rule(bs_params, df)
4. Updated example code to use bs_params parameter name
5. Added note about default_update being a bound method that becomes unbound
Changes to tests/test_bootstrap.py:
1. Updated test to use 'bs_params' instead of 'self' for consistency
2. This avoids confusion about whether it's a bound or unbound function
The key insight: When users define custom update_rule functions, they should use:
def my_update(bs_params, df):
bs_params.shared_args['key'] = value
NOT:
def my_update(self, df): # Confusing - looks like a method
self.shared_args['key'] = value
Addresses reviewer feedback about confusing documentation.
…sses The docstrings for the 'parent' attribute in Experiment and its subclasses had two issues: 1. **Missing quotes for forward reference**: The Experiment base class is defined before the stochastic_benchmark class, so the type annotation needs quotes to indicate a forward reference. 2. **Typo**: 'stochatic_benchmark' → 'stochastic_benchmark' (missing 's') Changes: - Experiment: parent : stochastic_benchmark → parent : 'stochastic_benchmark' - ProjectionExperiment: parent : stochatic_benchmark → parent : 'stochastic_benchmark' - StaticRecommendationExperiment: parent : stochastic_benchmark → parent : 'stochastic_benchmark' - RandomSearchExperiment: parent : stochatic_benchmark → parent : 'stochastic_benchmark' - SequentialSearchExperiment: parent : stochatic_benchmark → parent : 'stochastic_benchmark' Benefits: - Proper forward reference annotation using quotes - Fixed typos in class name - Consistent documentation across all Experiment subclasses - Clarified that parent refers to the stochastic_benchmark class (not the module) Addresses reviewer feedback about docstring type annotation clarity.
Addresses reviewer feedback about error message clarity when bs_results is None
vs. wrong type.
Changes to src/stochastic_benchmark.py:
1. **Improved None check error message** (lines 1481-1484):
- Changed from generic Exception to specific ValueError
- Added more descriptive error message explaining what went wrong
- Suggests calling Bootstrap() or initBootstrap() to fix the issue
2. **Removed redundant None checks** (lines 1488-1510 and 1637-1659):
- Removed redundant 'if self.bs_results is None' checks inside the
reduce_mem branches
- These were unnecessary since None is already checked earlier
- Keeps only the isinstance type checks with clear TypeError messages
3. **Improved TypeError messages**:
- Changed type(self.bs_results) to type(self.bs_results).__name__
for cleaner output
- Removed redundant 'but got...' from some messages for consistency
Benefits:
- **Clearer error messages**: Users get ValueError when bs_results is None (with
helpful guidance), separate from TypeError when it's the wrong type
- **Fail-fast behavior**: None check happens before type checking
- **Better debugging**: Error messages now distinguish between "forgot to run
Bootstrap" vs "wrong data type"
- **Removed code duplication**: Eliminated redundant None checks
Added tests in tests/test_smoke.py:
- TestStochasticBenchmarkErrorHandling class with 3 new tests:
1. test_interpolate_with_none_bs_results_clear_error
2. test_interpolate_reduce_mem_with_none_bs_results_clear_error
3. test_interpolate_with_wrong_type_gives_type_error
All tests verify that:
- ValueError is raised (not Exception) when bs_results is None
- TypeError is raised when bs_results has wrong type
- Error messages are clear and actionable
Addresses reviewer feedback about using logging instead of print() for consistency
with the rest of the codebase, and adds better context for error conditions.
Changes to examples/wishart_n_50_alpha_0.5/wishart_ws.py:
1. **Added logging import and setup** (lines 4, 18-19):
- Imported logging module
- Created logger instance using __name__
2. **Improved prepare_param error handling** (lines 420-425):
- Changed from silent 'return' to explicit 'return None' with logging
- Added logger.warning() with parameter context when res_list is empty
- Message indicates this may be due to empty resource_list or no experiments
- More informative for debugging data processing issues
3. **Improved concatenation error handling** (lines 444-469):
- Replaced print() statements with appropriate logging levels:
* logger.warning() for empty ret_list (potential issue)
* logger.info() for successful concatenation count
* logger.error() for concatenation exceptions
- Added explanatory comment about returning empty DataFrame
- Notes that this behavior may mask underlying problems
- Suggests considering raising an exception if this is unexpected
Benefits:
- **Consistent logging**: Uses logging module like other parts of codebase
- **Better diagnostics**: Logger includes parameter context for debugging
- **Appropriate log levels**: Warning/Error/Info used correctly
- **Documented behavior**: Comment explains empty DataFrame return trade-off
- **Maintainability**: Logging can be configured/filtered vs. print statements
The changes maintain backward compatibility (still returns empty DataFrame when
ret_list is empty) while providing better visibility into error conditions through
proper logging.
This reverts commit 8ffdfe8.
Create a comprehensive roadmap for addressing PEP 8 naming convention violations in a future major version release. The current codebase uses PascalCase for method names (e.g., run_Bootstrap, initAll) instead of the PEP 8 recommended snake_case. This is a deliberate design choice that has been consistent throughout the project, but should be addressed in a future v2.0 release to align with Python best practices. Document includes: - Complete inventory of naming violations - Phased migration plan with deprecation period - Dual API approach (recommended) - Migration tools and guides for users - Timeline estimates (6-9 months) - Risk mitigation strategies - Success criteria Recommended approach: Maintain dual API during v1.x with deprecation warnings, then clean cutover in v2.0. This minimizes disruption while achieving PEP 8 compliance. This is marked as a future enhancement and is NOT part of the current PR scope.
jvpcms
reviewed
Oct 27, 2025
jvpcms
reviewed
Oct 27, 2025
Collaborator
bernalde
commented
Nov 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the project to require Python 3.10+ and adds comprehensive improvements to testing infrastructure, documentation, and examples.
Changes
Python Version & Dependencies
CI/CD Infrastructure
Type Safety Improvements
bootstrap.pyandstochastic_benchmark.pyisinstance()instead oftype()for type checkingDocumentation & Examples
Code Quality
Testing
Closes #31 (supersedes the old PR that was closed due to branch recreation)