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

Not possible to pickle objects #329

Closed
berahtlv opened this issue Feb 12, 2021 · 8 comments · Fixed by #331
Closed

Not possible to pickle objects #329

berahtlv opened this issue Feb 12, 2021 · 8 comments · Fixed by #331

Comments

@berahtlv
Copy link

On pynetbox version higher than 5.0.8 it is not possible to pickle objects. Used for testing purposes in absence of Netbox.

pynetbox-5.3.1/pynetbox-5.1.1:

nbox = pynetbox.api(NBOX_URL, token=NBOX_TOKEN)
sites = nbox.dcim.sites.all()
f = open("object.pickle", "wb")
pickle.dump(sites, f)
Traceback (most recent call last):
File "", line 1, in
TypeError: 'App' object is not callable

pynetbox-5.0.8:

nbox = pynetbox.api(NBOX_URL, token=NBOX_TOKEN)
sites = nbox.dcim.sites.all()
f = open("object.pickle", "wb")
pickle.dump(sites, f)
f.close()

@markkuleinio
Copy link
Contributor

This is my attempted fix in PluginsApp in app.py:

    def __getstate__(self):
        return self.__dict__

    def __setstate__(self, d):
        self.__dict__.update(d)

(and it makes pickle.dump() work for me, even though I've never worked with pickle so YMMV)

but pytest does not work due to:

...
tests/integration/conftest.py:6: in <module>
    import yaml
E   ModuleNotFoundError: No module named 'yaml'

@raddessi or @zachmoody, is there something specific we should know nowadays when contributing? I've seen you developing the testing setup lately.

@raddessi
Copy link
Contributor

raddessi commented Feb 12, 2021

We're currently testing adding an integration suite to the project but unfortunately I think my first commit was not very stable and broke not long after netbox-docker was updated to 1.0.. I apologize. I think I have those issues sorted in #327 but it still needs a look over. As far as I know integration tests aren't needed yet but Zach will have to weigh in on path forward for now. I'm pretty sure this project should still support python2 but I'm not 100% sure to what extent.

@raddessi
Copy link
Contributor

raddessi commented Feb 12, 2021

@berahtlv What version of python are you running? You should be able to run the unit tests using python 3 without error, but the integration tests will fail right now I think. Try running python3 -m pytest tests/test* tests/unit/*

@berahtlv
Copy link
Author

I am running python 3.6.8 and Netbox v2.9.3. Issue is related to App instance pickling.
Yes, tests python3 -m pytest tests/test* tests/unit/* are successful.

@markkuleinio
Copy link
Contributor

@raddessi @zachmoody Note that the master branch is still broken:

(clean-venv)$ pytest
================================ test session starts =================================
platform linux -- Python 3.7.3, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
...
tests/integration/conftest.py:6: in <module>
    import yaml
E   ModuleNotFoundError: No module named 'yaml'

AFAIK yaml is not listed as a requirement anywhere. Should you be developing the integration tests in some other branch, or what do you suggest for other contributors to use as a base in this situation (or did I miss some change in contribution guidelines)?

@raddessi
Copy link
Contributor

Oh my apologies, for some reason I thought yaml was builtin in python 3.x now. We should probably add a requirements-dev.txt with yaml and the rest of the dev requirements listed in there in that case right?

@markkuleinio
Copy link
Contributor

The PR (#331) is currently passing the checks (in GitHub), so let's see if @zachmoody will accept it.

zachmoody pushed a commit that referenced this issue Apr 5, 2021
Add __getstate__ and __setstate__ methods in PluginsApp to fix #329
@zachmoody
Copy link
Contributor

Yeah, for sure, just merged it. Been trying to get things together for the 6.0 release. Wanted to put this one in the last 5.x release, but I'd already merged some of the breaking changes before I remembered this one was still outstanding 😬

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 a pull request may close this issue.

4 participants