search

openml / openml-python

Python module to interface with OpenML
https://openml.github.io/openml-python/main/
Other
279 stars 143 forks source link

Rework list_tasks: change raise error to warning for bad tasks to avoid crashing for bad server state #1244

Closed LennartPurucker closed 1 year ago

LennartPurucker commented 1 year ago

Closes #1234

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.12 :warning:

Comparison is base (bb3793d) 85.24% compared to head (09deee0) 85.12%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1244 +/- ## =========================================== - Coverage 85.24% 85.12% -0.12% =========================================== Files 38 38 Lines 5008 5009 +1 =========================================== - Hits 4269 4264 -5 - Misses 739 745 +6 ``` | [Impacted Files](https://codecov.io/gh/openml/openml-python/pull/1244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openml) | Coverage Δ | | |---|---|---| | [openml/tasks/functions.py](https://codecov.io/gh/openml/openml-python/pull/1244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openml#diff-b3Blbm1sL3Rhc2tzL2Z1bmN0aW9ucy5weQ==) | `85.32% <0.00%> (-0.47%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://codecov.io/gh/openml/openml-python/pull/1244/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openml) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openml). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openml)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mfeurer commented 1 year ago

Hey, is this still an issue? I don't think we should add workaround code for uncommon bugs on the server (i.e. they happened only once so far) and rather fix things on the server instead.

LennartPurucker commented 1 year ago

Hey, is this still an issue? I don't think we should add workaround code for uncommon bugs on the server (i.e. they happened only once so far) and rather fix things on the server instead.

Mhm, I would say the current workflow (i.e., crashing) is an issue, and the alternative workflow (i.e., warning instead of crashing) would be better. But I see your point that we technically are only following the server specification.

I would leave this to your judgment. Feel free to close the PR.

PGijsbers commented 1 year ago

I vote +1 on merging this.

Philosophically I agree that openml-python should not accommodate to all kind of buggy server states. However specifically for this case, I think the pragmatic approach to issue a warning instead of an error leads to a much better user experience. My reason for preferring this is because the error is more likely than not caused by tasks in which the user had no interest to begin with (to contrast, I would not add work-arounds for actually downloading and instantiating such a corrupted task, for example).

mfeurer commented 1 year ago

Merging this now as two people would like to have this merged.