Skip to content

Deprecate output_format='dict' - #1258

Merged
mfeurer merged 21 commits into
developfrom
pandas_default
Jun 16, 2023
Merged

Deprecate output_format='dict'#1258
mfeurer merged 21 commits into
developfrom
pandas_default

Conversation

@PGijsbers

@PGijsbers PGijsbers commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator

Deprecates output_format='dict':

  • add a future warning with any of the list_* functions are called with output_format='dict',
  • update all internal code and examples which used to rely on the default of output_format='dict'

WIP: Going through unit tests I incidentally broke

PGijsbers added 15 commits June 15, 2023 11:07
@PGijsbers
Add deprecation warning for retrieving dict
30dd55f
@PGijsbers
Refactor check_datasets_active to work with dataframe
b502312
@PGijsbers
Update unit tests to use list_datasets with output_format dataframe
357bb7d
@PGijsbers
Move list_datasets test to proper file
29bbb57
@PGijsbers
Remove list_datasets test, duplicate in test_datasets_functions
464e5dd 
duplicate of tests/test_datasets/test_dataset_functions.py::test_list_datasets
@PGijsbers
Update list_flows calls to use output_format='dataframe'
aaad25f
@PGijsbers
Update list_runs calls to require dataframe output
cf9dd7b
@PGijsbers
Update list_setup calls for deprecation
13f2fb5
@PGijsbers
Update list_study calls
d3342a1
@PGijsbers
Update list_tasks to specify output_format dataframe
b8a915b
@PGijsbers
Add output_format to list_datasets call
3361b15
@PGijsbers
Add TODO markers for removing dict support of list_* functions
be16355
@PGijsbers
Make status check less strict, call list_dataset with output_format
5cc1287
@PGijsbers
Change index on id to did, since thats the dataset id's column name
576e09c
@PGijsbers
Update test to reflect new error message
b82febe
@PGijsbers
PGijsbers requested a review from mfeurer June 15, 2023 14:16
mfeurer
mfeurer reviewed Jun 15, 2023
View reviewed changes
Comment thread openml/datasets/functions.py Outdated
Comment thread tests/test_utils/test_utils.py
@LennartPurucker

LennartPurucker commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

Looks good to me at this point.

@codecov-commenter

codecov-commenter commented Jun 15, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 86.20% and project coverage change: +3.84 🎉

Comparison is base (3b3553b) 81.66% compared to head (c130c41) 85.51%.

❗ Current head c130c41 differs from pull request most recent head 22a6dd3. Consider uploading reports for the commit 22a6dd3 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1258      +/-   ##
===========================================
+ Coverage    81.66%   85.51%   +3.84%     
===========================================
  Files           38       38              
  Lines         5012     5343     +331     
===========================================
+ Hits          4093     4569     +476     
+ Misses         919      774     -145
Impacted Files Coverage Δ
openml/study/functions.py 85.36% <25.00%> (-5.19%) ⬇️
openml/evaluations/functions.py 82.35% <50.00%> (+11.04%) ⬆️
openml/flows/functions.py 83.97% <50.00%> (-0.77%) ⬇️
openml/tasks/functions.py 84.73% <75.00%> (+1.04%) ⬆️
openml/datasets/dataset.py 87.55% <91.07%> (+1.77%) ⬆️
openml/datasets/functions.py 93.01% <93.93%> (+4.76%) ⬆️
openml/datasets/data_feature.py 71.42% <100.00%> (+2.19%) ⬆️
openml/runs/functions.py 84.13% <100.00%> (+8.71%) ⬆️
openml/setups/functions.py 93.08% <100.00%> (+7.14%) ⬆️
openml/utils.py 92.21% <100.00%> (+0.34%) ⬆️

... and 9 files with indirect coverage changes

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

@PGijsbers
PGijsbers marked this pull request as ready for review June 15, 2023 17:43
@PGijsbers

PGijsbers commented Jun 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

I think I cleared up the tests I accidentally broke after refactoring, but there are a lot of failures due to the test server state (also on develop). I would prefer to wait until we have working CI again.

LennartPurucker
LennartPurucker reviewed Jun 16, 2023
View reviewed changes
Comment thread tests/test_runs/test_run.py Outdated
Comment thread tests/test_tasks/test_task_methods.py Outdated
@mfeurer
mfeurer requested a review from LennartPurucker June 16, 2023 11:02
@PGijsbers

PGijsbers commented Jun 16, 2023

Copy link
Copy Markdown
Collaborator Author

@mfeurer only test_list_all_with_multiple_batches fails now, as expected. I think we can go ahead and merge this.

@mfeurer
mfeurer merged commit 495162d into develop Jun 16, 2023
@mfeurer
mfeurer deleted the pandas_default branch June 16, 2023 12:43
github-actions Bot pushed a commit that referenced this pull request Jun 16, 2023
Github Actions
Pieter Gijsbers: Deprecate output_format='dict' (#1258)
f57493d
@PGijsbers PGijsbers mentioned this pull request Aug 10, 2023
Open
AtharvaManale
AtharvaManale reviewed Oct 11, 2026
View reviewed changes

@AtharvaManale AtharvaManale left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey I have reviewed the commits as they explicitly handle the domino effect of changing output_format="dict" to "dataframe". Handling all effective/affected unit tests and functions of datasets, tasks, runs, flows, evalutions, setups ad study.

@PGijsbers

PGijsbers commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

As I said on the issue tracker; thanks so much for checking this! For next time, it suffices to say so on the issue tracker (especially since you linked to this PR anyway). Please refrain from commenting on long-closed PRs unless there is a specific issue with the PR (and even then, linking it from the still-open issue might be more useful for keeping the current conversation in one place).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfeurer mfeurer mfeurer approved these changes
@LennartPurucker LennartPurucker LennartPurucker approved these changes
+1 more reviewer
@AtharvaManale AtharvaManale AtharvaManale left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PGijsbers @LennartPurucker @codecov-commenter @mfeurer @AtharvaManale