Ticket #887 (closed bug: fixed)

Opened 3 years ago

Last modified 3 years ago

Unpickling of a Variable with a recursive structure can fail.

Reported by: ales Owned by: ales
Milestone: Future Component: other
Severity: major Keywords:
Cc: anze.staric@… Blocking:
Blocked By:

Description

For instance if

variable.get_value_from.class_var is variable

is True. In fact the pickle module refuses to even pickle it, but cPickle just ignores this and some times produces incorrect results (see example below and related  http://bugs.python.org/issue1062277). This could be fixed by moving the non essential members from the arguments for 'pickleLoaderVariable' to the state dictionary (if replaceVarWithEquivalent does not break because of this). Also other classes need to be checked for this. Replaces #875.

Example

import Orange, pickle, cPickle
data = Orange.data.Table("titanic")
data = Orange.preprocess.Preprocessor_continuize(data)
var = data.domain[0]
#pickle.dumps(var) raises an exception
pck = lambda obj: pickle.loads(cPickle.dumps(obj))
pck(var) # This works
pck([var, var]) # This also works
pck([var, [var]]) # This is [var, [<Recursion on list with id=...>]
pck([data.domain, data.domain.variables]) #Raises TypeError: expected 'orange.Variable', got 'dict'

Change History

comment:1 Changed 3 years ago by janez

  • Cc anze.staric@… added

Aleš, you're right, your fix:

PyObject *Variable__reduce__(PyObject *self)
{
	PyTRY
		PyObject *name = PyObject_GetAttrString(self, "name");
		PyObject *dict = packOrangeDictionary(self);
		PyMapping_SetItemString(dict, "name", name);
		Py_DECREF(name);
        PyObject *state = PyDict_New();
        PyObject *pygetvf = PyDict_GetItemString(dict, "get_value_from");
        if (pygetvf) {
            PyDict_SetItemString(state, "get_value_from", pygetvf);
            PyDict_DelItemString(dict, "get_value_from");
        }
	return Py_BuildValue("O(ON)N", getExportedFunction("__pickleLoaderVariable"), self->ob_type, dict, state); //packOrangeDictionary(self));
	PyCATCH
}

seems to work.

I guess that it shouldn't break replaceVarWithEquivalent since getValueFrom is checked (in isEquivalentTo) for equality only if it exists,

    !getValueFrom || !old.getValueFrom || (getValueFrom == old.getValueFrom)

Could you test and commit it? Anže, you can try it too, it should fix your precious LogRegClassifier. :) (I didn't know that the ticket I got sent by Aleš two weeks ago is related to that.)

By the way, in Orange 3.0 I use getnewargs and state dictionaries instead of reduce. Much cleaner.

comment:2 Changed 3 years ago by ales

  • Status changed from new to closed
  • Resolution set to fixed

In [12021]:

Moving get_value_from to the state dictionary in the Variable.reduce methods (in case it contains a recursive reference to the variable about to be pickled).
Fixes #887.

Note: See TracTickets for help on using tickets.