Skip to content

Fix 1013: Store run setup_string - #1015

Merged
mfeurer merged 11 commits into
developfrom
fix_1013
Mar 25, 2021
Merged

Fix 1013: Store run setup_string#1015
mfeurer merged 11 commits into
developfrom
fix_1013

Conversation

@PGijsbers

@PGijsbersPGijsbers commented Dec 21, 2020

Copy link
Copy Markdown
Collaborator

Closes#1013.

mfeurer
mfeurer reviewed Dec 21, 2020
View reviewed changes
Comment threadtests/test_runs/test_run_functions.py Outdated
@PGijsbers
Test setup_string in xml without roundtrip
65eb5c0
Also moved the test to OpenMLRun, since it mainly tests the OpenMLRun
behavior, not a function from openml.runs.functions.
PGijsbers
PGijsbers commented Dec 21, 2020
View reviewed changes
Comment threadopenml/runs/functions.py
@mfeurer

mfeurer commented Dec 21, 2020

Copy link
Copy Markdown
Collaborator

Hey, I think the changes are good, but it's really hard to tell given the massive amount of failing unit tests. We have fixed most of them in PR #1000, but it would be great if you could check the changes in _api_calls.py. We can then merge it and you could rebase this one so the tests are more indicative of whether the changes actually work?

@PGijsbersPGijsbers mentioned this pull request Jan 4, 2021
Closed
3 tasks
@PGijsbers

PGijsbers commented Jan 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

I noticed description_text is also not stored, will add.

@PGijsbers

PGijsbers commented Jan 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

I don't see description in the xsd, but there is run_details: https://github.com/openml/OpenML/blob/develop/openml_OS/views/pages/api_new/v1/xsd/openml.run.upload.xsd
Do we use that?

@PGijsbers

PGijsbers commented Jan 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Oops. Looks like description_text relates to the arff description. Adding run_details separately.

mfeurer
mfeurer reviewed Mar 22, 2021
View reviewed changes
Comment threadopenml/runs/run.py
@PGijsbers
PGijsbers requested a review from mfeurerMarch 24, 2021 12:58
@PGijsbers
Remove pipeline code from setup_string
6f9215a
Long pipelines (e.g. gridsearches) could lead to too long setup strings.
This prevented run uploads.
Also add mypy ignores for old errors which weren't yet vetted by mypy.
@PGijsbers
Merge branch 'fix_1013' ofhttps://github.com/openml/openml-pythonin…
f084587
…to fix_1013
@PGijsbers

PGijsbers commented Mar 25, 2021

Copy link
Copy Markdown
CollaboratorAuthor

@mfeurer I think it's done now. Updated the progress log so it doesn't need a separate PR. The extension.py file apparently hadn't been made mypy compliant yet, so I had to add some ignores (I wasn't sure why the attributes weren't recognized, but I don't have time to sort that out right now- I hope that's ok).

@mfeurer
mfeurer merged commit bd8ae14 into developMar 25, 2021
@mfeurer
mfeurer deleted the fix_1013 branch March 25, 2021 20:55
github-actionsBot pushed a commit that referenced this pull request Mar 25, 2021
Github Actions
PGijsbers: Fix 1013: Store runsetup_string(#1015)
d1238c8
PGijsbers added a commit to Mirkazemi/openml-python that referenced this pull request Feb 23, 2023
@PGijsbers
Fix 1013: Store runsetup_string(openml#1015)
aa33a98
* Test setup_string is stored and retrievable
* Add setup_string to run dictionary representation
* Add fix to release notes
* Test setup_string in xml without roundtrip
Also moved the test to OpenMLRun, since it mainly tests the OpenMLRun
behavior, not a function from openml.runs.functions.
* Serialize run_details
* Update with merged PRs since 11.0
* Prepare for run_details being provided by the server
* Remove pipeline code from setup_string
Long pipelines (e.g. gridsearches) could lead to too long setup strings.
This prevented run uploads.
Also add mypy ignores for old errors which weren't yet vetted by mypy.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfeurermfeurerAwaiting requested review from mfeurer

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

setup_string of Run is not saved

3 participants

@PGijsbers@mfeurer@joaquinvanschoren