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

Remove marshmallow from result/ and requirements #4030

Merged
merged 24 commits into from
May 7, 2020

Conversation

mtreinish
Copy link
Member

Summary

This commit finishes the process of removing marshmallow from terra. It
rebuilds the result class as bare python classes or SimpleNamespace
subclasses (for classes that allow arbitrary fields). After the result
class has been converted there is nothing using marshmallow or all the
support code in qiskit/validation. So this continues the removal process
by removing those and removing marshmallow and marshmallow-polyfield
from the requirements list.

Details and comments

This commit depends on #4027 and #4016 and can't be merged (or tested) without
those applied.

@mtreinish mtreinish added the on hold Can not fix yet label Mar 26, 2020
@mtreinish mtreinish force-pushed the remove-marshmallow-forever branch from 1734e57 to dd3a458 Compare March 26, 2020 22:55
This commit finishes the process of removing marshmallow from terra. It
rebuilds the result class as bare python classes or SimpleNamespace
subclasses (for classes that allow arbitrary fields). After the result
class has been converted there is nothing using marshmallow or all the
support code in qiskit/validation. So this continues the removal process
by removing those and removing marshmallow and marshmallow-polyfield
from the requirements list.

This commit depends on Qiskit#4027 and Qiskit#4016 and can't be merged without
those.
@mtreinish mtreinish force-pushed the remove-marshmallow-forever branch from dd3a458 to c542e11 Compare March 26, 2020 23:01
@mtreinish mtreinish added this to the 0.14 milestone Mar 26, 2020
@1ucian0 1ucian0 marked this pull request as draft April 16, 2020 15:01
While both Result and ExperimentResult take arbitrary key value date via
the kwargs using SimpleNamespace as a parent class like in other places
is not a good fit. This is because the signatures for these classes are
signfiicantly different from the base SimpleNamespace. So when Results
are pickled via async execution (like in the BasicAer and Aer providers)
this causes it to either fail or if a custom __reduce__ method is
defined it to not work as expected. This commit pivots the new classes
to be bare objects that store the arbitrary kwargs as a private
dictionary attribute. Attribute access for these fields are implemented
via a custom __getattr__.
The results tests were using a raw Marshmallow.Obj object in the tests
to construct the Result qobj headers and relying on the schema to
convert it to a QobjExperimentHeader when it was passed to the Result
constructor. When marshmallow was removed this was updated to a dict()
since that was the closest mapping, but that ignored the transform
marshmallow would do. This commit corrects that oversight and
constructs QobjExperimentHeader object where dict was incorrectly used
before. This is probably what the original tests should have done for
clarity anyway.
@mtreinish mtreinish changed the title [WIP] Remove marshmallow from result/ and requirements Remove marshmallow from result/ and requirements Apr 16, 2020
@mtreinish mtreinish marked this pull request as ready for review April 16, 2020 15:44
@mtreinish
Copy link
Member Author

mtreinish commented Apr 16, 2020

This is still on hold because this will break the ibmq-provider until Qiskit/qiskit-ibmq-provider#553 is fixed. Once that's done we can safely merge this. (this is also what's blocking the docs builds because the ibmq provider fails to import with this applied)

@mtreinish mtreinish requested a review from jaygambetta as a code owner April 21, 2020 11:52
@kdk kdk modified the milestones: 0.14, 0.15 Apr 21, 2020
jyu00
jyu00 previously approved these changes May 5, 2020
Copy link
Contributor

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

A couple minor comments but overall looks good!

qiskit/validation/__init__.py Outdated Show resolved Hide resolved
qiskit/result/result.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

This looks fine. The only question I have is do we make it clear that to_dict and from_dict are designed to serialize/deserialize JSON? It might be possible to the wrong idea from the function name alone.

@mtreinish
Copy link
Member Author

Honestly, I'm not sure if that's a concern for this PR. Those methods are there for backwards compatibility with marshmallow only, if I could I would have just removed them. Ideally if I were to add json support to these classes I would have done it with to_json/from_json (or maybe dump_json) or probably even better would just be save and load and had it actually do the serialization. In practice though I don't know if people (outside of the ibmq provider) are actually using it with json, the places I would expect it to be used don't (like ignis for saving experimental results relies on pickles and ignores them). Other (non-ibmq) providers typically actually do from_dict but not from loading json instead from a manually constructed dict, like:

mostly because the marshmallow based classes had poor documentation and if you were going to construct a Results object using the constructors it relied on three layers of nested class construction so from_dictwas much easier.

@jyu00
Copy link
Contributor

jyu00 commented May 6, 2020

The to_dict and from_dict don't really do JSON (de)serialization, as they don't convert non-JSON-serializable objects (e.g. complex) to JSON serializable. They really just convert an object to its dictionary representation.

@mergify mergify bot merged commit e115252 into Qiskit:master May 7, 2020
@mtreinish mtreinish deleted the remove-marshmallow-forever branch May 7, 2020 12:26
@mtreinish mtreinish added Changelog: API Change Include in the "Changed" section of the changelog Changelog: Removal Include in the Removed section of the changelog labels Aug 3, 2020
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* Remove marshmallow from result/ and requirements

This commit finishes the process of removing marshmallow from terra. It
rebuilds the result class as bare python classes or SimpleNamespace
subclasses (for classes that allow arbitrary fields). After the result
class has been converted there is nothing using marshmallow or all the
support code in qiskit/validation. So this continues the removal process
by removing those and removing marshmallow and marshmallow-polyfield
from the requirements list.

This commit depends on Qiskit#4027 and Qiskit#4016 and can't be merged without
those.

* Fixes from testing

Now Qiskit#4016 has merged this is unblocked on the terra side. This commit
makes some changes and fixes issues found from testing.

* Abandon SimpleNamespace for Result and ExperimentResult

While both Result and ExperimentResult take arbitrary key value date via
the kwargs using SimpleNamespace as a parent class like in other places
is not a good fit. This is because the signatures for these classes are
signfiicantly different from the base SimpleNamespace. So when Results
are pickled via async execution (like in the BasicAer and Aer providers)
this causes it to either fail or if a custom __reduce__ method is
defined it to not work as expected. This commit pivots the new classes
to be bare objects that store the arbitrary kwargs as a private
dictionary attribute. Attribute access for these fields are implemented
via a custom __getattr__.

* Use correct header type in tests

The results tests were using a raw Marshmallow.Obj object in the tests
to construct the Result qobj headers and relying on the schema to
convert it to a QobjExperimentHeader when it was passed to the Result
constructor. When marshmallow was removed this was updated to a dict()
since that was the closest mapping, but that ignored the transform
marshmallow would do. This commit corrects that oversight and
constructs QobjExperimentHeader object where dict was incorrectly used
before. This is probably what the original tests should have done for
clarity anyway.

* Fix lint

* Fix docstring

* Add release notes

* Fix docs issues from review comments

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 26, 2020
In the removal of marshmallow from the Result class (and terra as a
whole) in Qiskit#4030 the descriptive repr for the objects was lost. This is
inconvenient because the default representation for an object is just
the class name and the memory address of the object. This commit adds
back a string representation of the objects which were missing by adding
a __repr__ method to the classes.
mergify bot pushed a commit that referenced this pull request Oct 5, 2020
* Add back repr for Result and related classes

In the removal of marshmallow from the Result class (and terra as a
whole) in #4030 the descriptive repr for the objects was lost. This is
inconvenient because the default representation for an object is just
the class name and the memory address of the object. This commit adds
back a string representation of the objects which were missing by adding
a __repr__ method to the classes.

* Fix lint

* Add test

* Fix lint

Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Removal Include in the Removed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants