[ENH] Replace asserts with proper if else Exception handling#1589
[ENH] Replace asserts with proper if else Exception handling#1589Omswastik-11 wants to merge 14 commits intoopenml:mainfrom
asserts with proper if else Exception handling#1589Conversation
|
Hi @fkiraly !! I would like to know you suggestion on this .
In |
asserts with proper if else error handlingasserts with proper if else Exception handling
fkiraly
left a comment
There was a problem hiding this comment.
Yes, looks reasonable for me for a first go, to isolate the if/else in a function.
More generally, from an architecture standpoint however, whenever I see a long list of if/elses on the argument class type, I think it is too high coupling and it should be either a method of the (task/argument) class or a visitor pattern.
Why: imagine you want to add a new task. Now you have to add another elif in all of these functions. This is absolutely not extensible.
This should be refactored - I would say, in another PR, after opening an issue with a plan - so the task itself, or a visitor, manages whatever is in the if/else.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
==========================================
- Coverage 52.82% 52.72% -0.10%
==========================================
Files 37 37
Lines 4371 4381 +10
==========================================
+ Hits 2309 2310 +1
- Misses 2062 2071 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
asserts with proper if else Exception handlingasserts with proper if else Exception handling
geetu040
left a comment
There was a problem hiding this comment.
Good, just ran the tests in tests/test_runs/ to be sure of the refactor, and it's green.
Ready to be merged!
There was a problem hiding this comment.
Pull request overview
This PR replaces assert-based validation in the openml.runs module with explicit exceptions, improving reliability (asserts can be stripped with Python -O) and providing clearer failure modes. It also refactors ARFF attribute construction into a dedicated helper to reduce duplication.
Changes:
- Replaced multiple
assert ... is not Nonevalidations withValueError/TypeErrorraises across runs-related code paths. - Added
OpenMLRun._get_arff_attributes_for_task()and used it from_generate_arff_dict()to centralize ARFF attribute generation. - Tightened runtime type checks for inputs derived from task data (
ytype in parallel helper).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
openml/runs/trace.py |
Replaces asserts with explicit exceptions when parameters are unexpectedly missing. |
openml/runs/run.py |
Adds a helper for task-dependent ARFF attributes; replaces flow-id assertion with explicit error. |
openml/runs/functions.py |
Replaces several asserts with explicit exceptions in run/model initialization and run-list parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Fixes #1581