Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3261 #3262

Merged
merged 7 commits into from Nov 8, 2023
Merged

Conversation

CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented Nov 8, 2023

This should fix #3261.

There was some problems with the line in Component where a trait was tried to pass to t.Property. I'm not sure if this a change to the Traits API or some random bug that was never picked up.

Anyways custom components should serialize now.

    active = t.Property(t.CBool) --> active = t.Property() 
    name = t.Property(t.Str('')) --> name = t.Property()

@CSSFrancis CSSFrancis changed the base branch from RELEASE_next_minor to RELEASE_next_major November 8, 2023 19:19
@CSSFrancis
Copy link
Member Author

Just a note that my linter also picked this up a the wrong type for the t.Property() function based on the documentation for the traits package. :)

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4cafc47) 80.78% compared to head (41856f1) 80.78%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           RELEASE_next_major    #3262   +/-   ##
===================================================
  Coverage               80.78%   80.78%           
===================================================
  Files                     140      140           
  Lines                   20396    20400    +4     
  Branches                 4825     4826    +1     
===================================================
+ Hits                    16476    16480    +4     
- Misses                   2845     2846    +1     
+ Partials                 1075     1074    -1     
Files Coverage Δ
hyperspy/component.py 89.83% <100.00%> (+0.04%) ⬆️
hyperspy/model.py 85.78% <100.00%> (+0.21%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CSSFrancis CSSFrancis mentioned this pull request Nov 8, 2023
57 tasks
@ericpre
Copy link
Member

ericpre commented Nov 8, 2023

Thank you @CSSFrancis!

I have tried adding the following test and I still can't get it saving custom component to work, with dill or cloudpickle:

def test_loading_non_expression_custom_component():
    # non-expression based custom component uses serialisation
    # to save the components.
    """
    The file has been generated with hyperspy 2.0 using:

        import hyperspy.api as hs
        from hyperspy.component import Component

        class CustomComponent(Component):

            def __init__(self, p1=1, p2=2):
                Component.__init__(self, ('p1', 'p2'))

                self.p1.value = p1
                self.p2.value = p2

                self.p1.grad = self.grad_p1
                self.p2.grad = self.grad_p2

            def function(self, x):
                p1 = self.p1.value
                p2 = self.p2.value
                return p1 + x * p2

            def grad_p1(self, x):
                return 0

            def grad_p2(self, x):
                return x


        s = hs.signals.Signal1D(range(10))
        m = s.create_model()

        c = CustomComponent()
        m.append(c)
        m.store('a')

        s.save("hs2.0_custom_component.hspy")

    """

    s = hs.load(DIRPATH / "hs2.0_custom_component.hspy")
    _ = s.models.restore('a')

@CSSFrancis
Copy link
Member Author

CSSFrancis commented Nov 8, 2023

@ericpre this appears to be working on my computer using this branch using the code use tested above.

Edit: It appears to fail when running the tests... Now I'm a little confused...

@ericpre
Copy link
Member

ericpre commented Nov 8, 2023

Edit: It appears to fail when running the tests... Now I'm a little confused...

I have been there! 😉

@CSSFrancis
Copy link
Member Author

CSSFrancis commented Nov 8, 2023

@ericpre Looks like this is something related to python 3.11. I was using two environments one of which was 3.10 and one which was 3.11.

@CSSFrancis
Copy link
Member Author

I also found this: python/cpython#100316

@CSSFrancis
Copy link
Member Author

This appears to have been fixed by cloudpipe/cloudpickle#466 but for some reason we still see the error.

@ericpre
Copy link
Member

ericpre commented Nov 8, 2023

For future reference the error is:

_________________ test_loading_non_expression_custom_component _________________
[...]
    
        s = hs.load(DIRPATH / "hs2.0_custom_component.hspy")
>       _ = s.models.restore('a')

hyperspy/tests/component/test_component.py:364: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
hyperspy/signal.py:325: in restore
    return self._signal.create_model(dictionary=copy.deepcopy(d))
hyperspy/_signals/signal1d.py:393: in create_model
    model = Model1D(self, dictionary=dictionary)
hyperspy/models/model1d.py:277: in __init__
    self._load_dictionary(dictionary)
hyperspy/model.py:383: in _load_dictionary
    self.append(reconstruct_component(comp, **init_args))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

comp_dictionary = {'_active_array': None, '_class_dump': b'\x80\x05\x95\x08\x0b\x00\x00\x00\x00\x00\x00\x8c\x17cloudpickle.cloudpickle\x...ponent', '_whitelist': {'_active_array': '', '_id_name': '', 'active': '', 'active_is_multidimensional': '', ...}, ...}
init_args = {}, _id = 'CustomComponent'

    def reconstruct_component(comp_dictionary, **init_args):
        # Restoring of Voigt and Arctan components saved with Hyperspy <v1.6
        if (comp_dictionary['_id_name'] == "Voigt" and
                len(comp_dictionary['parameters']) > 4):
            # in HyperSpy 1.6 the old Voigt component was moved to PESVoigt
            if comp_dictionary['parameters'][4]['_id_name'] == "resolution":
                comp_dictionary['_id_name'] = "PESVoigt"
                _logger.info("Legacy Voigt component converted to PESVoigt during file reading.")
        if (comp_dictionary['_id_name'] == "Arctan" and 'minimum_at_zero' in comp_dictionary):
            # in HyperSpy 1.6 the old Arctan component was moved to EELSArctan
            if comp_dictionary['minimum_at_zero'] == True:
                comp_dictionary['_id_name'] = "EELSArctan"
                _logger.info("Legacy Arctan component converted to EELSArctan during file reading.")
        _id = comp_dictionary['_id_name']
        if _id in _COMPONENTS:
            _class = getattr(
                importlib.import_module(
                    _COMPONENTS[_id]["module"]), _COMPONENTS[_id]["class"])
        elif "_class_dump" in comp_dictionary:
            # When a component is not registered using the extension mechanism,
            # it is serialized using cloudpickle.
>           _class = cloudpickle.loads(comp_dictionary['_class_dump'])
E           TypeError: code() argument 13 must be str, not int

hyperspy/model.py:128: TypeError

@CSSFrancis
Copy link
Member Author

CSSFrancis commented Nov 8, 2023

@ericpre That was a fun set of bugs to track down :) I think the issue is related to saving using a previous version of python. Python/cloudpickle doesn't support pickling between different versions of python and as a result with the change in 3.11 reading of pickled classes from 3.10 and below seems to not work anymore.

I don't really think there is a great way around this other than raising an error and pointing people towards creating an extension? It seems difficult to even suggest a python version to use unless we save that in the metadata.

Edit: Now the test suite should pass. I messed up my greater than less than :) Also I have no clue if this will pass in python 3.12

@ericpre ericpre merged commit 6163e53 into hyperspy:RELEASE_next_major Nov 8, 2023
23 checks passed
@ericpre
Copy link
Member

ericpre commented Nov 8, 2023

Edit: Now the test suite should pass. I messed up my greater than less than :) Also I have no clue if this will pass in python 3.12

Most likely that it will not but this can be sorted when python 3.12 is added to the test suite. We can do this after the rc release!

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.

Serialisation of custom component fails
2 participants