Can't deepcopy() a ticket?

[Chris Nelson]

In my TracPM module, I want to give schedulers free reign over ticket
data. I used to do:

def computeSchedule(self, options, tickets):
# Convert list to dictionary, making copies so schedule can
# mess with the tickets.
ticketsByID = {}
for t in tickets:
ticketsByID[t['id']] = t

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

but changes made by the scheduler were carried back to the caller. I
started with:

def computeSchedule(self, options, tickets):
# Convert list to dictionary, making copies so schedule can
# mess with the tickets.
ticketsByID = {}
for t in tickets:
ticketsByID[t['id']] = copy.copy(t)

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

# Copy back the schedule results
for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

But the ticket attritbutes weren't copied, only the references to them.
I tried but deepcopy() but that gives:

File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line
558, in _add_tasks
self.pm.computeSchedule(options, self.tickets)
File "build/bdist.linux-x86_64/egg/tracjsgantt/tracpm.py", line 566,
in computeSchedule
ticketsByID[t['id']] = copy.deepcopy(t)
File "/usr/lib/python2.6/copy.py", line 162, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.6/copy.py", line 255, in _deepcopy_dict
y[deepcopy(key, memo)] = deepcopy(value, memo)
File "/usr/lib/python2.6/copy.py", line 189, in deepcopy
y = _reconstruct(x, rv, 1, memo)
File "/usr/lib/python2.6/copy.py", line 322, in _reconstruct
args = deepcopy(args, memo)
File "/usr/lib/python2.6/copy.py", line 162, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.6/copy.py", line 235, in _deepcopy_tuple
y.append(deepcopy(a, memo))
File "/usr/lib/python2.6/copy.py", line 189, in deepcopy
y = _reconstruct(x, rv, 1, memo)
File "/usr/lib/python2.6/copy.py", line 323, in _reconstruct
y = callable(*args)
TypeError: __init__() takes exactly 3 arguments (1 given)

Is this a known restriction of some Trac classes? A bug? My misuse of
deepcopy()?

This is in Trac 0.11.6.

Any insight appreciated.

Chris
--
Christopher Nelson, Software Engineering Manager
SIXNET - Solutions for Your Industrial Networking Challenges
331 Ushers Road, Ballston Lake, NY 12019
Tel: +1.518.877.5173, Fax: +1.518.877.8346 www.sixnet.com

[Chris Nelson]

> ... But the ticket attritbutes weren't copied, only the references to them.

>   I tried but deepcopy() but that gives:
>
>    File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line
> 558, in _add_tasks
>      self.pm.computeSchedule(options, self.tickets)
>    File "build/bdist.linux-x86_64/egg/tracjsgantt/tracpm.py", line 566,
> in computeSchedule
>      ticketsByID[t['id']] = copy.deepcopy(t)
>    File "/usr/lib/python2.6/copy.py", line 162, in deepcopy
>      y = copier(x, memo)
>    File "/usr/lib/python2.6/copy.py", line 255, in _deepcopy_dict
>      y[deepcopy(key, memo)] = deepcopy(value, memo)
>    File "/usr/lib/python2.6/copy.py", line 189, in deepcopy
>      y = _reconstruct(x, rv, 1, memo)
>    File "/usr/lib/python2.6/copy.py", line 322, in _reconstruct
>      args = deepcopy(args, memo)
>    File "/usr/lib/python2.6/copy.py", line 162, in deepcopy
>      y = copier(x, memo)
>    File "/usr/lib/python2.6/copy.py", line 235, in _deepcopy_tuple
>      y.append(deepcopy(a, memo))
>    File "/usr/lib/python2.6/copy.py", line 189, in deepcopy
>      y = _reconstruct(x, rv, 1, memo)
>    File "/usr/lib/python2.6/copy.py", line 323, in _reconstruct
>      y = callable(*args)
>    TypeError: __init__() takes exactly 3 arguments (1 given)

> ...

http://grokbase.com/p/python.org/python-list/2009/01/how-to-deepcopy-a-list-of-user-defined-lists/08o7x4coflamidhtsvod2ewoluna
notes:

A subclass of list should populate the list in __init__, not __new__,
usually by calling list.__init__, as lists are mutable and subclasses
thereof should be too.

I don't believe TracPM has any subclass of list and I think Ticket
maybe which suggests to me there's something wrong in Trac core. With
a little support, I'll work on the patch but core is unfamiliar to me.

[osimons]

On Jan 12, 2:25 pm, Chris Nelson <Chris.Nel...@SIXNET.com> wrote:
> > ... But the ticket attritbutes weren't copied, only the references to them.
> >   I tried but deepcopy() but that gives:

> >    TypeError: __init__() takes exactly 3 arguments (1 given)
> > ...
>

> http://grokbase.com/p/python.org/python-list/2009/01/how-to-deepcopy-...

> notes:
>
>  A subclass of list should populate the list in __init__, not __new__,
> usually by calling list.__init__, as lists are mutable and subclasses
> thereof should be too.
>
> I don't believe TracPM has any subclass of list and I think Ticket
> maybe which suggests to me there's something wrong in Trac core.  With
> a little support, I'll work on the patch but core is unfamiliar to me.

And 'tickets' in your code snippet is what exactly - a list of trac
ticket objects? If so, they are objects with various internals that
you can not depend on copy or deepcopy to handle. The
trac.ticket.model.Ticket class uses __getitem__ and __setitem__ to
appear as dict, but the actual dict with the information is stored in
t.values.

A Ticket object may also have objects of other classes attached, like
self.resource (from trac.resource.Resource) and having it 'copied'
will either be impossible, wrong, uncertain or all those things...
However, when classes have __copy__() and __deepcopy__() methods,
those could contain the logic for the object to reliably copy itself
in a way that accounts for all its intricate internals. See
http://docs.python.org/library/copy.html (at end of page).

Lesson: Don't copy or deepcopy objects unless either you or the class
know exactly what needs to be done.


:::simon

https://www.coderesort.com
http://trac-hacks.org/wiki/osimons

[Chris Nelson]

On 01/12/2012 09:14 AM, osimons wrote:
> On Jan 12, 2:25 pm, Chris Nelson<Chris.Nel...@SIXNET.com> wrote:
>>> ... But the ticket attritbutes weren't copied, only the references to them.
>>> I tried but deepcopy() but that gives:
>>> TypeError: __init__() takes exactly 3 arguments (1 given)
>>> ...
>>
>> http://grokbase.com/p/python.org/python-list/2009/01/how-to-deepcopy-...
>> notes:
>>
>> A subclass of list should populate the list in __init__, not __new__,
>> usually by calling list.__init__, as lists are mutable and subclasses
>> thereof should be too.
>>
>> I don't believe TracPM has any subclass of list and I think Ticket
>> maybe which suggests to me there's something wrong in Trac core. With
>> a little support, I'll work on the patch but core is unfamiliar to me.
>
> And 'tickets' in your code snippet is what exactly - a list of trac
> ticket objects?

Yes.

> If so, they are objects with various internals that
> you can not depend on copy or deepcopy to handle. The
> trac.ticket.model.Ticket class uses __getitem__ and __setitem__ to
> appear as dict, but the actual dict with the information is stored in
> t.values.

> ...

> Lesson: Don't copy or deepcopy objects unless either you or the class
> know exactly what needs to be done.

Thanks for the explanation. I'll find another way.

Which leads me to ask why:

# Remember original pred, succ values
pred = {}
succ = {}
for tid in ticketsByID:
pred[tid] = copy.copy(ticketsByID[tid]['pred'])
succ[tid] = copy.copy(ticketsByID[tid]['succ'])

# Update pred, succ as needed for scheduling
_augmentTickets(ticketsByID)

for id in ticketsByID:
if options['schedule'] == 'alap':
_schedule_task_alap(ticketsByID[id])
else:
_schedule_task_asap(ticketsByID[id])

# Put original pred, succ back
for tid in ticketsByID:
ticketsByID[tid]['pred'] = pred[tid]
ticketsByID[tid]['succ'] = succ[tid]

leaves the scheduler changes to pred and succ visible to the caller.

[osimons]

Chris, your code looks way over-complicated for whatever it seems to
try to do. That said I cannot safely say I understand your use case..
However, I can elaborate on the original topic of making a copy of the
ticket values:

I left you a clue when I said that t.values will contain all the
actual ticket data - the strings, datateime and integers that makes up
the ticket data. As these are all immutable values, there is no need
to deepcopy - a straight copy would suffice:

ticket_values = {}
for t in tickets:
ticket_values[t.id] = t.values.copy()

Or if you don't want to store it in a dict with ID as key but instead
need to retain the order in a list, you need to include the ticket id
in the values as it is not stored in values. So this should make a
simplified copy of the tickets with just the values:

ticket_values = []
for t in tickets:
t_vals = t.values.copy()
t_vals['id'] = t.id
ticket_values.append(t_vals)

OK?

[Chris Nelson]

On 01/12/2012 10:35 AM, osimons wrote:
> ...

> Chris, your code looks way over-complicated for whatever it seems to
> try to do.

That may be. I'm not the strongest Python programmer in the world.

> That said I cannot safely say I understand your use case..

He's my use case. TracPM defines

class ITaskScheduler(Interface):
# Schedule each the ticket in ticketsByID with consideration for
# dependencies, estimated work, hours per day, etc.
#
# ticketsByID is a dictionary, indexed by numeric ticket ID, each
# ticket contains at least the fields returned by queryFields()
# and the whole list was processed by postQuery().
#
# On exit, each ticket has t['calc_start'] and t['calc_finish']
# set (FIXME - we should probably be able to configure those field
# names.) and can be accessed with TracPM.start() and finish().
def scheduleTasks(self, options, ticketsByID):
"""Called to schedule tasks"""

and

# tickets is an unordered list of tickets. Each ticket contains
# at least the fields returned by queryFields() and the whole list
# was processed by postQuery().
def computeSchedule(self, options, tickets):

computeSchedule() takes a list of tickets because that's the natural way
for other code to handle the data (as returned from a query, used to
display a Gantt, etc.) ITaskScheduler takes a dictionary because it had
to create one interinally anyway and I can have computeSchedule() build
it for all schedulers and use that as an opportunity to isolate the
caller of computeSchedule() from the vagaries of the scheduler (which
might want to update tickets in weird ways).

So, my implementation of computeSchedule is:

# Convert list to dictionary, making copies so schedule can
# mess with the tickets.

ticketsByID = {}
for t in tickets:
# FIXME - deepcopy fails here but with only copy, if the
# scheduler updates fields, those updates are visible to
# the caller.

ticketsByID[t['id']] = copy.copy(t)

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

# Copy back the schedule results
for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

but it doesn't work, the caller still sees the changes to other fields
that the scheduler makes.

> However, I can elaborate on the original topic of making a copy of the
> ticket values:
>
> I left you a clue when I said that t.values will contain all the
> actual ticket data - the strings, datateime and integers that makes up
> the ticket data.

I guess my clue detector is on the fritz. Sorry.

> As these are all immutable values, there is no need
> to deepcopy - a straight copy would suffice:
>
> ticket_values = {}
> for t in tickets:
> ticket_values[t.id] = t.values.copy()
>
> Or if you don't want to store it in a dict with ID as key but instead
> need to retain the order in a list, you need to include the ticket id
> in the values as it is not stored in values. So this should make a
> simplified copy of the tickets with just the values:
>
> ticket_values = []
> for t in tickets:
> t_vals = t.values.copy()
> t_vals['id'] = t.id
> ticket_values.append(t_vals)

So, something like:

# tickets is an unordered list of tickets. Each ticket contains
# at least the fields returned by queryFields() and the whole list
# was processed by postQuery().

def computeSchedule(self, options, tickets):
# Convert list to dictionary, making copies so schedule can
# mess with the tickets.

ticketsByID = {}
for t in tickets:
ticketsByID[t['id']] = t.values.copy()

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

# Copy back the schedule results
for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

?

[Chris Nelson]

On 01/12/2012 10:49 AM, Chris Nelson wrote:
> ...

> So, something like:
>
> # tickets is an unordered list of tickets. Each ticket contains
> # at least the fields returned by queryFields() and the whole list
> # was processed by postQuery().
> def computeSchedule(self, options, tickets):
> # Convert list to dictionary, making copies so schedule can
> # mess with the tickets.
> ticketsByID = {}
> for t in tickets:
> ticketsByID[t['id']] = t.values.copy()
>
> # Schedule the tickets
> self.scheduler.scheduleTasks(options, ticketsByID)
>
> # Copy back the schedule results
> for t in tickets:
> for field in [ 'calc_start', 'calc_finish']:
> t[field] = ticketsByID[t['id']][field]
>
> ?

Which gives me:

File "build/bdist.linux-x86_64/egg/tracjsgantt/tracpm.py", line 563,
in computeSchedule

ticketsByID[t['id']] = t.values.copy()

AttributeError: 'builtin_function_or_method' object has no attribute 'copy'

[Ethan Jucovy]

On Thu, Jan 12, 2012 at 10:57 AM, Chris Nelson <Chris....@sixnet.com> wrote:

ticketsByID[t['id']] = t.values.copy()

 

Which gives me:

AttributeError: 'builtin_function_or_method' object has no attribute 'copy'

The error message is telling you that ``t.values`` is a function.  So you need to call it to get a copyable data structure:

t.values().copy()

Based on that, it looks like ``t`` may already be a dict instead of a trac Ticket object.  (Probably it's the result of accessing ticket.values somewhere.)  If so you can create a copy of the dict by simply saying ``t.copy()`` or ``dict(t)`` (the latter is idiomatic)

To double check you might want to throw in a ``import pdb; pdb.set_trace()`` in your code and check ``type(t)`` to make sure the object is what you think it is.

[osimons]

Yes, something like that looks like it should work. But if done like
that, I still don't understand why you wouldn't just pass the tickets
to your scheduler? Your code comments say you pass a copy so that
schedulers can "mess" with the ticket data, but any Trac and plugin
code should really be trusted to do the right thing with objects they
get passed. I don't see the reason for building structures to hide and
protect data when all is available anyway. It is Python after all...
But, your code - you decide :-)


:::simon

[osimons]

On Jan 12, 5:07 pm, Ethan Jucovy <ethan.juc...@gmail.com> wrote:

> On Thu, Jan 12, 2012 at 10:57 AM, Chris Nelson <Chris.Nel...@sixnet.com>wrote:
>
> > ticketsByID[t['id']] = t.values.copy()
>
> > Which gives me:
> > AttributeError: 'builtin_function_or_method' object has no attribute 'copy'
>
> The error message is telling you that ``t.values`` is a function.

Right, so 'tickets' is actually not a list of ticket objects... Chris,
it really makes sense for your code to follow the common Trac practice
of passing actual objects around instead of mutating them into various
structures. It just makes it so much harder for everyone, and makes
code less readable and questions harder to anwser. In everything I
read in your question and code, t should be a trac.ticket.model.Ticket
object - and 'tickets' should be a list of them....


:::simon

[Ethan Jucovy]

On Thu, Jan 12, 2012 at 11:13 AM, osimons <odds...@gmail.com> wrote:

Right, so 'tickets' is actually not a list of ticket objects... Chris,
it really makes sense for your code to follow the common Trac practice
of passing actual objects around instead of mutating them into various
structures. It just makes it so much harder for everyone, and makes
code less readable and questions harder to anwser. In everything I
read in your question and code, t should be a trac.ticket.model.Ticket
object - and 'tickets' should be a list of them....

Tracing through the code where ``tickets`` is defined ( http://trac-hacks.org/browser/tracjsganttplugin/0.11/tracjsgantt/tracjsgantt.py#L546 ) I see that it's actually the result of a trac.ticket.query.Query().execute() call, which returns a list of raw dicts: http://trac.edgewall.org/browser/trunk/trac/ticket/query.py#L326

Does a higher-level or more formal API exist in Trac core for fetching tickets from a query?  I remember looking for one a while ago and not finding anything.

[Chris Nelson]

On 01/12/2012 11:07 AM, Ethan Jucovy wrote:
> On Thu, Jan 12, 2012 at 10:57 AM, Chris Nelson <Chris....@sixnet.com
> <mailto:Chris....@sixnet.com>> wrote:
>
> ticketsByID[t['id']] = t.values.copy()
>
> Which gives me:
> AttributeError: 'builtin_function_or_method' object has no attribute
> 'copy'
>
>
> The error message is telling you that ``t.values`` is a function. So you
> need to call it to get a copyable data structure:
>
> t.values().copy()

Which doesn't work because t.values() returns a list which doesn't have
a copy function. So I do

copy.copy(t.values())

only to find out that that gives me a list of raw values without keys
which makes it rather useless to me.

> Based on that, it looks like ``t`` may already be a dict instead of a
> trac Ticket object. (Probably it's the result of accessing ticket.values
> somewhere.)

No, I've never done that; didn't know of Ticket.values() until today.

> If so you can create a copy of the dict by simply saying
> ``t.copy()`` or ``dict(t)`` (the latter is idiomatic)

--

[Chris Nelson]

On 01/12/2012 11:09 AM, osimons wrote:
> ...

> Yes, something like that looks like it should work. But if done like
> that, I still don't understand why you wouldn't just pass the tickets
> to your scheduler? Your code comments say you pass a copy so that
> schedulers can "mess" with the ticket data, but any Trac and plugin
> code should really be trusted to do the right thing with objects they
> get passed. I don't see the reason for building structures to hide and
> protect data when all is available anyway. It is Python after all...
> But, your code - you decide :-)

It's not hiding details from the scheduler but giving the scheduler the
freedom to update values as needed for scheduling (whatever the
algorithm may need) and keeping the contract with the caller of
computeSchedule() that the only difference is that calc_start and
calc_finish have been added to each ticket.

I could have each scheduler cache values it may stop on and put them
back before returning (though I can't get copy to work there, either).
Or I could have each scheduler make a scratch copy of any ticket
attributes it needs to change and only work with that copy (though that
seems less elegant in many ways).

Perhaps one way to think of it is that the scheduler operates on "tasks"
and a "task" is a dictionary that holds the same values as a ticket. To
the extent that ticket['fieldname'] works, duck typing says that a
ticket is a task but because I can't deep copy a ticket to preserve its
attributes, that breaks down.

Chris

[Ethan Jucovy]

On Thu, Jan 12, 2012 at 1:18 PM, Chris Nelson <Chris....@sixnet.com> wrote:
> Which doesn't work because t.values() returns a list which doesn't have a
> copy function.  So I do
>
>  copy.copy(t.values())
>
> only to find out that that gives me a list of raw values without keys which
> makes it rather useless to me.

Yeah. What you want is actually:

[Chris Nelson]

On 01/12/2012 11:13 AM, osimons wrote:
> On Jan 12, 5:07 pm, Ethan Jucovy<ethan.juc...@gmail.com> wrote:
>> On Thu, Jan 12, 2012 at 10:57 AM, Chris Nelson<Chris.Nel...@sixnet.com>wrote:
>>
>>> ticketsByID[t['id']] = t.values.copy()
>>
>>> Which gives me:
>>> AttributeError: 'builtin_function_or_method' object has no attribute 'copy'
>>
>> The error message is telling you that ``t.values`` is a function.
>
> Right, so 'tickets' is actually not a list of ticket objects...

If it isn't, I don't know where it stopped being one. Or maybe it never
was, I'm taking the results of something like `Query('id=23|45') and
handling each row as a "ticket". On thinking about it, I guess that
doesn't give me a list of ticket objects, it gives me a list of
dictionaries with each dictionary holding attributes of a ticket. Which
is fine for my purposes but leaves me wondering why I can't deepcopy the
resulting dictionary.

> ...

[Chris Nelson]

But even

def computeSchedule(self, options, tickets):
# Convert list to dictionary, making copies so schedule can
# mess with the tickets.
ticketsByID = {}
for t in tickets:

self.env.log.debug('%s values are "%s"' %
(t['id'], dict(t)))
ticketsByID[t['id']] = copy.copy(dict(t))

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

# Copy back the schedule results
for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

leaves changes to ticketsByID visible in tickets to the caller of
computeSchedule(). And if I do `copy.deepcopy(dict(t))` I'm back to
`__init__() takes exactly 3 arguments (1 given)`.

[Ethan Jucovy]

On Thu, Jan 12, 2012 at 1:31 PM, Chris Nelson <Chris....@sixnet.com> wrote:
> But even
[snip]

>            ticketsByID[t['id']] = copy.copy(dict(t))

[snip]

>        # Copy back the schedule results
>        for t in tickets:
>            for field in [ 'calc_start', 'calc_finish']:
>                t[field] = ticketsByID[t['id']][field]
>
> leaves changes to ticketsByID visible in tickets to the caller of
> computeSchedule().  And if I do `copy.deepcopy(dict(t))` I'm back to
> `__init__() takes exactly 3 arguments (1 given)`.

Huh. Then it sounds like you need to drop into an interactive pdb
session in there to inspect the dict and figure out exactly what
nested object is involved. Once you isolate that you can just
special-case it one way or another.

[Chris Nelson]

I actually don't need ticket objects (and I'm sorry for my sloppy
nomenclature; I know if makes communication difficult). My PM stuff
(e.g., scheduling) only acts on ticket attributes and having there
results of a query is fine and adequate.

computeSchedule() should be easy to call with those query results:

# tickets is an unordered list of tickets. Each ticket contains
# at least the fields returned by queryFields() and the whole list
# was processed by postQuery().

#
# On return, each element of tickets has had two fields added:
# calc_start, and calc_finish. No other changes are made.
def computeSchedule(self, options, tickets):

But the scheduler is going to have to look up ticket values by ID again
and again, Each scheduler implementation could do it's own lookup table
but I put it in the wrapper:

# Convert list to dictionary

ticketsByID = {}
for t in tickets:

ticketsByID[t['id']] = copy.copy(dict(t))

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

To give the scheduler the freedom to change attributes or add others, I
copy only the intended fields back from the result.

# Copy back the schedule results
for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

But nothing I do seems to isolate the caller from scheduler changes to
the dependency fields (and possibly others but that's what I can see in
my current failure case).

Chris

[Chris Nelson]

For now

# tickets is an unordered list of tickets. Each ticket contains
# at least the fields returned by queryFields() and the whole list
# was processed by postQuery().

def computeSchedule(self, options, tickets):
# Convert list to dictionary, making copies so schedule can
# mess with the tickets.
ticketsByID = {}
for t in tickets:

ticketsByID[t['id']] = {}
for field in t:
ticketsByID[t['id']][field] = copy.copy(t[field])

# Schedule the tickets
self.scheduler.scheduleTasks(options, ticketsByID)

# Copy back the schedule results

for t in tickets:
for field in [ 'calc_start', 'calc_finish']:
t[field] = ticketsByID[t['id']][field]

works and I can clean it up later.

Thanks to all for their patience.

Chris