Converted dat file to json file to remove XGBoost versioning issues. #304
Conversation
|
Cool! We should also change This PR is currently going to master (not development); just pointing that out in case it doesn't match @mdeceglie's release plan. |
|
|
|
@mdeceglie ran into that a while ago, check the thread in the rdtools teams channel from Aug 27. Seems like he fixed it by installing from conda; maybe we should do the same for the OS X tests. Those tests were passing before though, so maybe for some reason it's caused by the new versions you're specifying in setup.py and requirements.txt? Are those changes necessary for some other reason or can we revert them? |
|
Oh, it's the rdtools/.github/workflows/requirements.yaml Lines 24 to 26 in 600991d We could just remove that step -- it seemed like a good idea to include, but that step isn't actually necessary for the job to fulfill its purpose, so I'd say let's just remove it if it's causing problems. @kperrynrel, want to delete those workflow lines and see what happens here? We might have to do something with conda if we pursue #303, but we can cross that bridge when we come to it. |
|
@kanderso-nrel removed the rdtools import and it's all passing. Let me know what you guys think! |
setup.py
Outdated
| @@ -45,7 +45,7 @@ | |||
| 'h5py >= 2.7.1', | |||
| 'plotly>=4.0.0', | |||
| 'joblib >= 0.16.0', | |||
| 'xgboost >= 1.3.3, <1.5.0', | |||
| 'xgboost >= 1.5.1', | |||
There was a problem hiding this comment.
What's the reason for updating the minimum? I thought the motivation for switching to JSON was so that we wouldn't have to worry about keeping track of xgboost versions. 1.3.3 seems to work for me locally with the JSON model, so maybe we don't actually need to change the version to 1.5.1 here and in the two requirements files? So long as it's not a headache for us it's nice to not be overly restrictive on dependency version ranges, so keeping 1.3.3 would be preferable if it doesn't break anything.
|
@kanderso-nrel I just updated it in a previous push when I was trying to debug the failing rdtools import. I can drop back down again, it'll work |
There was a problem hiding this comment.
LGTM thanks @kperrynrel! I'll defer to @mdeceglie for what he wants to do about v2.1.2, changelog file, etc
|
Oh, can we remove |
|
As an integration test, I installed this in a fresh environment and ran |
|
@kanderso-nrel successfully removed joblib in new commit |
|
@mdeceglie I did redownload the package in the new environment and was able to reproduce your results: |
|
Hey all, I just wanted to keep things simple, so I resaved the old model in the new JSON format and replicated the original results in the jupyter notebook: |
* update citation (#300) * update sphinx citation * less specific * update readme with new citation * capitalize Zenodo * further capitalization * Add DOI links to supporting refs * DOI fix * even more doi fixing * Converted dat file to json file to remove XGBoost versioning issues. (#304) * Converted dat file to json file. * Updated the xgboost versions to no latest version restriction. * Updated the model loading for xgb to handle multiple XGB versions. * debugging mac os XGB error?? * bumped xgb to lower version to handle mac OS * update versioning. * more xgboost version updates... * updated the workflows script to remove rdtools pip install. * dropped xgboost version down * updated the XGB model pull with os.path.join() * fixed pep8 issues. * Removed the joblib requirement per @kanderso's request. * Regenerated the old model and saved as a json. * run notebook for cell numbering Co-authored-by: Perry <kperry@nrel.gov> Co-authored-by: Michael Deceglie <Michael.Deceglie@nrel.gov> * Update changelog * double backticks in 2.1.1 log * add cell 2 back in * adjust changelog * add PR to a changelog entry Co-authored-by: Kirsten Perry <70228568+kperrynrel@users.noreply.github.com> Co-authored-by: Perry <kperry@nrel.gov>
Code changes are covered by testsNew functions added to__init__.pyAPI.rst is up to date, along with other sphinx docs pagesUpdated changelog