Skip to content

Conversation

@jdebacker
Copy link
Member

This PR updates the OG-USA Specifications() class to inherit the ParamTools.parameters() class.


def test_update_specification_with_json():
spec = Specifications()
new_spec_json = """
Copy link
Member Author

Choose a reason for hiding this comment

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

@hdoupe This test is not passing because the JSON is not specified properly:

if raise_errors and self._errors:
>           raise self.validation_error
E           paramtools.exceptions.ValidationError: {
E               "frisch": [
E                   "Not a valid number: {'value': [{'value': 0.3}]}."
E               ]
E           }

../../ParamTools/paramtools/parameters.py:244: ValidationError

What is the proper format to use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work:

    new_spec_json = """
    {
        "frisch": [{"value": 0.3}]
    }
    """

Since you're not using labels, you can simplify it to this:

new_spec_json = """
{
    "frisch": 0.3
}
"""

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it - thanks @hdoupe.

@jdebacker
Copy link
Member Author

@hdoupe When running OG-USA, I've been pickling the Specifications object so that I can easily retrieve parameters from each model run. I'm noticing that when Specifications is built off of paramtools.parameters, that I cannot pickle the object because of the dependency on marshmallow:

>           pickle.dump(spec, open(param_dir, "wb"))
E           _pickle.PicklingError: Can't pickle <class 'marshmallow.schema.DefaultsSchema'>: attribute lookup DefaultsSchema on marshmallow.schema failed

execute.py:88: PicklingError

Is that right? Any work around?

@hdoupe
Copy link
Contributor

hdoupe commented Sep 16, 2019

@jdebacker I found the source of the pickle problem, but I'm not sure how to solve it yet. I opened an issue in Paramtools: PSLmodels/ParamTools#78

I'll try to get a fix up for this today.

@hdoupe
Copy link
Contributor

hdoupe commented Sep 16, 2019

@jdebacker what do you think about using cloudpickle for dumping your Python objects? It should already be installed since dask lists it as a dependency, and it serializes python objects that Python's pickle does not.

The issue is that ParamTools dynamically creates Marshmallow classes dynamically using the type function, and Python's pickle only supports classes that were defined at run-time. Stackoverflow has a ton of questions related to this (one, two, three). One solution showed how you can define your own method for pickling in this situation, but it didn't work very well, and I'm a little nervous about supporting something like that even if I can get it to work.

@jdebacker
Copy link
Member Author

@hdoupe Thanks for looking into the issue of being able to pickle the class object. I hadn't heard of cloudpickle before, but it sounds useful. This statement gives me pause:

Using cloudpickle for long-term object storage is not supported and strongly discouraged.

I'll do some more reading...

@codecov-io
Copy link

codecov-io commented Sep 16, 2019

Codecov Report

Merging #494 into master will decrease coverage by 0.72%.
The diff coverage is 81.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
- Coverage   64.96%   64.23%   -0.73%     
==========================================
  Files          46       45       -1     
  Lines        5546     5366     -180     
==========================================
- Hits         3603     3447     -156     
+ Misses       1943     1919      -24
Impacted Files Coverage Δ
ogusa/__init__.py 100% <ø> (ø) ⬆️
ogusa/tests/test_TPI.py 38.92% <ø> (ø) ⬆️
ogusa/tests/test_txfunc.py 67.24% <0%> (-0.59%) ⬇️
ogusa/tests/test_parameters.py 90.74% <100%> (-9.26%) ⬇️
ogusa/execute.py 15.21% <14.28%> (+0.58%) ⬆️
ogusa/parameters.py 51.88% <83.33%> (-8.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33467f...3967c91. Read the comment docs.

@hdoupe
Copy link
Contributor

hdoupe commented Sep 16, 2019

Ah, I agree. That statement was added in this PR cloudpipe/cloudpickle#127 in response to cloudpipe/cloudpickle#123. If you specify the default protocol, pickle.DEFAULT_PROTOCOL, in cloudpickle.dump, then it seems like cloudpickle will use a safer, more compatible protocol for creating the pickle file. I think things should work like normal once the file is dumped, since cloudpickle just uses the regular Python pickle for loading pickle files.

@jdebacker
Copy link
Member Author

@hdoupe Ok - let me switch things to save the parameters class object with cloudpickle. I'm reassured that it only takes pickle to open the files and that cloudpickle will be installed with dask. Thanks for this suggestion.

@hdoupe
Copy link
Contributor

hdoupe commented Sep 16, 2019

I'm reassured that it only takes pickle to open the files and that cloudpickle will be installed with dask.

That's what I was thinking, too. If there are any shortcomings or you want to drop the use of cloudpickle at some point in the future, I'm happy to revisit this issue.

@jdebacker
Copy link
Member Author

@rickecon This PR passes all local tests and is ready for your review.

@rickecon
Copy link
Member

The failing CI checks are all:

E   ModuleNotFoundError: No module named 'ogusa.parametersbase'

stemming from the test*.py files that cause the ogusa/__init__.py file to run, which has the following import.

from ogusa.parametersbase import *

It looks like ogusa/parametersbase.py was deleted in this PR. So I think the fix is as simple as deleting that import line in the offending ogusa/__init__.py file. I just submitted a PR to your open PR with this fix. See if it passes the CI after integrating.

@jdebacker
Copy link
Member Author

@rickecon Thanks for that PR. I removed a few other references to the old parametersbase.py from the docs and package building files also. I think this is now ready.

@rickecon rickecon merged commit e31ec9f into PSLmodels:master Sep 19, 2019
@jdebacker jdebacker mentioned this pull request Sep 20, 2019
@jdebacker jdebacker deleted the paramtools branch November 13, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants