Merge into trunk : batch_update : Code : desktopcouch

Status:	Merged
Approved by:	Chad Miller on 2010-02-25
Approved revision:	not available
Merged at revision:	not available
Proposed branch:	lp:~mandel/desktopcouch/batch_update
Merge into:	lp:desktopcouch
Diff against target:	67 lines (+46/-0) 2 files modified desktopcouch/records/server_base.py (+26/-0) desktopcouch/records/tests/test_server.py (+20/-0)
To merge this branch:	bzr merge lp:~mandel/desktopcouch/batch_update
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
John O'Brien (community)		Approve on 2010-01-25
Stuart Langridge (community)		Approve on 2010-01-25
Eric Casteleijn (community)	2010-01-15	Approve on 2010-01-16
Review via email: mp+17482@code.launchpad.net

Revision history for this message

Manuel de la Peña (mandel) wrote on 2010-01-15:

#

The patch adds a put_records_batch method that puts a batch of docs in the db and a test that ensures that works as expected. The docs are added using a single request while the attachments are added one by one.

The result is a tuple of the form ``(success, docid, rev_or_exc)``, where ``success`` is a boolean indicating whether the update succeeded, ``docid`` is the ID of the document, and ``rev_or_exc`` is either the new document revision, or an exception instance (e.g. `ResourceConflict`) if the update failed.

Use case: Add a group of contacts to a category/tag

Concerns: If an attachment fails the doc are not rolled back. This same behavior if found in the single put_record method.

Revision history for this message

Eric Casteleijn (thisfred) wrote on 2010-01-16:

#

Looks good to me, and tests pass. Of course raising exceptions would be preferable to returning them, but in this case couchdb itself makes that hard to impossible.

review: Approve

Revision history for this message

Stuart Langridge (sil) wrote on 2010-01-18:

#

Download full text (19.5 KiB)

Man, I don't like returning an exception instance. After a long discussion on #python, though, I'm not sure there's a better way. Entire discussion pasted below (sorry, it's quite long).

<aquarius> If I've got a function which does multiple things, some of which may fail, but failure of some things doesn't mean that the others didn't succeed, what's the best way of signalling that failure? If I raise a ItPartiallyWorked exception, then I'll still need to pass back the successful results (from the successful actions) in that exception too, which seems a bit wrong
<lvh> aquarius, a warning sounds in order
<lvh> aquarius, just return a tuple that might have None in some bits
<lvh> also, the "warning" module
<aquarius> lvh, well, that's what we're doing at the moment: we return a list of results, where each result is either a successful result or an exception. But returning a list with exception instances in it seems weird. I mean, we could throw a warning *too* to alert people that it happened, of course.
<lvh> aquarius, you probably want to split the separate bits up into smaller functions that *do* do the right thing and raise an exception, but aren't taxed with the extra problem of still delivering partial results, like the composing function
<Alberth> aquarius: output a warning line to stderr ?
<aquarius> lvh, ah, I can't split it up into little bits because I'm calling an external non-Python thing which actually does all the actions :)
<lvh> aquarius, yeah no, don't use exception objects as sentinel values
<lvh> aquarius, use None instead, your users will hate you less
<aquarius> lvh, so I pass all the actions in one go to the external thing (CouchDB, in this case), and it gives me back a list of successes or failures.
<lvh> aquarius, does the absolute order matter? IE is the fact that something is at position 2 meaningful?
<aquarius> if I use None, though, there's no indication of *why* that action failed.
<lvh> aquarius, or is it just the relative position (2 comes before 3 and after 1)
<aquarius> lvh, absolute order does not matter (the list that comes back is *actually* a list of tuples (id_of_action, success_or_failure)
<Alberth> aquarius, lvh: you can also use a dict {'stepA': 'ok', 'stepB': 'foo failed'}
<lvh> if you raise an exception inside a generator and you only catch it outside of it, is the state lost?
<lvh> the logical thing to assume would be yes, let me give it a shot
<aquarius> Alberth, yes. that's what we do. What should "foo failed" be, though? I mean, it could just be a string, but then what I've done is reinvented string Exceptions :)
<lvh> aquarius, sentinel value
<lvh> aquarius, typically yourmodule.FAILURE or None
<lvh> of course, FAILURE = None # at module scope works fine
<aquarius> lvh, a sentinel value won't tell me what type of failure it was, though, it'll just tell me that it did fail. I'd like to know why
<Alberth> nice idea, a set of constant values could work
<Alberth> NO_FOO=1 ; NO_BAR =2
<aquarius> but that's just exceptions reinvented!
<lvh> aquarius, no, return values do not travel up the stack
<Alberth> exceptions are used to stop execution imho
<lvh> well, they do: but only once
<Alberth> that is n...

Man, I don't like returning an exception instance. After a long discussion on #python, though, I'm not sure there's a better way. Entire discussion pasted below (sorry, it's quite long).

<aquarius>	If I've got a function which does multiple things, some of which may fail, but failure of some things doesn't mean that the others didn't succeed, what's the best way of signalling that failure? If I raise a ItPartiallyWorked exception, then I'll still need to pass back the successful results (from the successful actions) in that exception too, which seems a bit wrong
<lvh>	aquarius, a warning sounds in order
<lvh>	aquarius, just return a tuple that might have None in some bits
<lvh>	also, the "warning" module
<aquarius>	lvh, well, that's what we're doing at the moment: we return a list of results, where each result is either a successful result or an exception. But returning a list with exception instances in it seems weird. I mean, we could throw a warning *too* to alert people that it happened, of course.
<lvh>	aquarius, you probably want to split the separate bits up into smaller functions that *do* do the right thing and raise an exception, but aren't taxed with the extra problem of still delivering partial results, like the composing function
<Alberth>	aquarius: output a warning line to stderr ?
<aquarius>	lvh, ah, I can't split it up into little bits because I'm calling an external non-Python thing which actually does all the actions :)
<lvh>	aquarius, yeah no, don't use exception objects as sentinel values
<lvh>	aquarius, use None instead, your users will hate you less
<aquarius>	lvh, so I pass all the actions in one go to the external thing (CouchDB, in this case), and it gives me back a list of successes or failures.
<lvh>	aquarius, does the absolute order matter? IE is the fact that something is at position 2 meaningful?
<aquarius>	if I use None, though, there's no indication of *why* that action failed.
<lvh>	aquarius, or is it just the relative position (2 comes before 3 and after 1)
<aquarius>	lvh, absolute order does not matter (the list that comes back is *actually* a list of tuples (id_of_action, success_or_failure)
<Alberth>	aquarius, lvh: you can also use a dict {'stepA': 'ok', 'stepB': 'foo failed'}
<lvh>	if you raise an exception inside a generator and you only catch it outside of it, is the state lost?
<lvh>	the logical thing to assume would be yes, let me give it a shot
<aquarius>	Alberth, yes. that's what we do. What should "foo failed" be, though? I mean, it could just be a string, but then what I've done is reinvented string Exceptions :)
<lvh>	aquarius, sentinel value
<lvh>	aquarius, typically yourmodule.FAILURE or None
<lvh>	of course, FAILURE = None # at module scope works fine
<aquarius>	lvh, a sentinel value won't tell me what type of failure it was, though, it'll just tell me that it did fail. I'd like to know why
<Alberth>	nice idea, a set of constant values could work
<Alberth>	NO_FOO=1  ; NO_BAR =2
<aquarius>	but that's just exceptions reinvented!
<lvh>	aquarius, no, return values do not travel up the stack
<Alberth>	exceptions are used to stop execution imho
<lvh>	well, they do: but only once
<Alberth>	that is not what you do here, here you want to continue, and return a collection of results upwards
<lvh>	right
<aquarius>	so if there are fifty separate ways it can fail, and for the "do one action" function I've defined fifty different exceptions, I now need to define fifty equivalent sentinel values as well so they can be returned by the multiple-actions function? this seems...less than efficient.
<lvh>	aquarius, if you have fifty different ways it can fail it shouldn't be one giant black box
<lvh>	aquarius, chopping it up into tiny bits was the first thing I suggested for a reason ;-)
<aquarius>	lvh, why? urllib.urlopen can fail in fifty different ways for exactly the same reason; the remote web server can throw loads of different HTTP errors, the network might not be there, etc.
<lvh>	aquarius, and only a few of those are meaningful to catch
<aquarius>	lvh, I understand the point about chopping it up, and you are welcome to call do_one_action twenty times and trap the exception, but the whole point of the multiple-actions thing is precisely that you can call it once with twenty things, it calls the external process (CouchDB) once with all twenty things, and it'll tell you whether each succeeded or failed. It's for efficiency.
<lvh>	aquarius, and for each of the twenty things twenty *distinct* problems can happen?
<aquarius>	lvh, could do, yes.
<lvh>	aquarius, then I don't understand how your function works at all
<lvh>	aquarius, if you're making an n-ary version of do_one_action i would expect the n-ary to not return significantly different exceptions
<lvh>	besides perhaps TypeError (the thing you gave me isn't iterable)
<aquarius>	lvh, CouchDB is a database whose API is HTTP. So to store a new document in it you PUT/POST the document contents to a URL specific to that document. A document PUT may succeed, or it may fail because it conflicts with an existing document or because it violates the rules about what a document may contain in many different ways, or because the server is down.
<lvh>	aquarius, I know CouchDB
<aquarius>	lvh, ah, ok. so do_many_actions wraps Couch's bulk update function.
<lvh>	aquarius, right -- but none of those errors are *distinct*
<lvh>	aquarius, all of the failures that happen on the n-ary can also happen on a single update, right?
<aquarius>	correct.
<lvh>	aquarius, now it's making much more sense
<aquarius>	but they can happen individually to each component of an update, and they may happen to some components and not others
<aquarius>	we use validation, so that a document must conform to a specific schema (for instance, a "contact" document must have a name
<lvh>	aquarius, twisted had to solve a similar problem with Deferreds and Failures
<aquarius>	lvh, violating that validation throws an error specific to the violation ("a contact record must have a name field"), for example
<lvh>	aquarius, sentinel values is how you do it, sorry
<aquarius>	lvh, so I don't want to have to define a sentinel value for every possible failure, since there are *loads*, and we already have exceptions for this
<lvh>	well, twisted didn't really do it with a sentinel value, but twisted.python.failure.Failure isn't really what you need
<aquarius>	what's actually the problem with passing back an exception instance, other than inelegance?
<lvh>	aquarius, because then you're using instances as sentinel values
<lvh>	aquarius, the point of sentinel values is that everyone uses the exact same object
<lvh>	which is why None is so popular: it's a guaranteed singleton
<aquarius>	yeah, but...so? I don't really care whether I can deduce that row5 and row12 failed for the same reason. I never need to test equivalence between sentinels, which is why I don't think that sentinels are the right way :)
<Alberth>	if you don't need equivalence, why not use a boolean?
<lvh>	aquarius, exceptions travel up the stack until caught
<aquarius>	Alberth, because a boolean won't tell me *why* it failed, just that it did
<aquarius>	lvh, only if they're raised.
<lvh>	traveling up the stack is not what you want, because you store state at the bottom of it
<lvh>	aquarius, I've already told you why the only thing you do with exceptions is raise
<Alberth>	aquarius: how are you deciding that without comparing them?
<lvh>	aquarius, how would *returning* an exception instance be different from using a sentinel value
<lvh>	besides being more useless because sentinel values are typically used because you know everyone's using the same one
<aquarius>	Alberth, I can compare exception_instance.reason, which is a string, if I want to; that's effectively a sentinel.
<lvh>	aquarius, sure, but why does it need to be an exception to do that?
<Alberth>	pass the string up instead?
<lvh>	Alberth, I'm not sure I like that, strings are a data structure of last resort
<Alberth>	NO_FOO="no foo" ; NO_BAR="no bar"
<aquarius>	Alberth, yep, that's what I thought of, but passing back a string instead of a more complex data structure means that it's hard to pass additional data. This is (one of the reasons) why string exceptions went away.
<lvh>	aquarius, I don't understand why you want it to be an Exeption rather than any other type
<lvh>	*Exception
<lvh>	aquarius, I understand your argument for complex data structures and I agree
<lvh>	aquarius, twisted needed that and that resulted in twisted.python.failure.Failure
<aquarius>	lvh, it doesn't have to be an Exception. It just seemed a bit silly to me to create some other type MySignallingOfAnErrorThing in fifty different varieties when I'll use them in a very similar way to Exceptions, and when I already have Exceptions defined for all these things
<aquarius>	seems...wasteful.
<lvh>	aquarius, <aquarius> yeah, but...so? I don't really care whether I can deduce that row5 and row12 failed for the same reason. I never need to test equivalence between sentinels, which is why I don't think that sentinels are the right way :) # could you explain that in more detail? I'm not sure I understand
<lvh>	also, I don't agree with "when I'll use them in a very similar way to Exceptions"
<lvh>	you're using them in a way that isn't like an exception at all
<Alberth>	both lvh and I believe that an exception object should be used only for raise/catch, and not as data value.
<lvh>	actually, come to think of it, if you write your thing as a generator twisted.python.failure.Failure.throwExceptionIntoGenerator might be *exactly* what you want
<aquarius>	imagine that, say, if PUTting a new record to Couch results in a NoNameDefined problem. My put_one_record function, if Couch throws that problem, would raise a NoNameDefined exception and I want to tell the punter that. So calling it would be "try: put_one_record(data); except NoNameDefined: error_list.add("You didn't define a name")
<lvh>	aquarius, presumably append and not add, but I get what you're saying
<lvh>	aquarius, basically you want to be able to preserve generator state despite uncaught exceptions
<aquarius>	the put_many_records would do something very similar: "results=put_many_records(list_of_data); for result in results: if result.problem == NoNameDefinedSentinelValue: error_list.add("You didn't define a name for %s" % result.id)
<lvh>	aquarius, but I'm pretty sure you can't do that without ugly hacks or a time machine
<lvh>	or twisted.python.failure.Failure.throwExceptionIntoGenerator
<aquarius>	those two bits of code are way similar, it's just that one uses exceptions and the other doesn't. that's what I mean by "using them similarly"
<aquarius>	(I used "add" 'cos I'm thinking of error_list as a Gtk list display widget or something, but that's not important :))
<aquarius>	yeah. I couldn't really think of a decent way to do this; it's all a bit hacky, hence coming here in case there was something cool I don't know about :)
<lvh>	aquarius, twisted.python.failure.Failure.throwExceptionIntoGenerator is something cool and you probably didn't know about it
<aquarius>	the twisted thing might be good, but...is it independent, or do I have to turn my app into a Twisted app to get at it?
<aquarius>	lvh, already reading about it ;)
<lvh>	aquarius, pretty much everything in twisted.python can be used in 3rd party non-twisted apps
<lvh>	BUT Failure is explicitly designed for asynchronous error handling because exceptions aren't good at it
<lvh>	aquarius, in all honesty you're probably better off doing what you're doing now, except don't make the things you're returning Exceptions
<lvh>	either use sentinel values, or, if you need to pass more data, custom types
<lvh>	if you don't really care *which* row failed, use custom types
<lvh>	errr, sentinel values
<aquarius>	lvh, that's the conclusion I'm coming to...you still think that I should define fifty different ThingThatIsSortaLikeAnExceptionButNotReallys, one for each Exception I've got?
<lvh>	aquarius, I think you probably shouldn't have fifty
<lvh>	aquarius, and I think you probably shouldn't have the n-ary thing in your API
<lvh>	especially if it makes error handling this difficult
<lvh>	why don't people just do a for x in L: try: do_one_thing(x); except UglyError: continue?
<aquarius>	lvh, it's in couch's API for a reason, though, and that's that making one HTTP call is way, way more efficient if you're trying to update 200 documents at once.
<aquarius>	let alone 20000 :-)
<lvh>	aquarius, if you're convinced you need that, sentinel values or frame inspection
<aquarius>	at the moment you precisely do have to call do_one_thing 20000 times; we're wrapping couch's bulk_update API now, which is why we're trying to work out how to do it in a way that doesn't suck :)
<aquarius>	frame inspection?
<lvh>	aquarius, you should probably forget I suggested that
<aquarius>	every time I've manually crawled up the stack and looked at frames I've felt guilty about it and six months later I haven't been able to understand why I did it, but that doesn't mean that there's not some case somewhere where it's actually a good idea ;)
<lvh>	aquarius, anyway, the reaosn i didn't want exception instances is because you need to be able to figure out WHICH ONE it is at runtime
<lvh>	which is why identiy in sentinel values is a good idea
<aquarius>	WHICH ONE in what sense? I mean, I can tell what type of exception it is, no?
<lvh>	aquarius, how? typechecking?
<lvh>	a dispatch dict is way cleaner
<lvh>	or a sequence of "if e is mymodule.QUANTUM_TRANSMOGRIFIER_EXPLOSION:"
<aquarius>	lvh, I can have a dispatch dict keyed with Exception subclasses, though, no? and then dispatch on isinstance
<Alberth>	aquarius: instead of trying one big do-everything-in-one-call, why not split it in phases?  make an object that collects sub-queries you have to do (as sub-data objects), a single 'do-it' that takes the collected sub-queries, and does them together, and stores the exec results back in the sub-queries in the sub-data objects, then investigate each sub-query object for the result afterwards?
<lvh>	aquarius, no, you can't dispatch inside a dictionary based on a boolean value
<lvh>	aquarius, you can do it with type instead of isinstance, but then you're using the type in order to get to a sentinel value
<aquarius>	lvh, yeah, I just realised that my idea's stupid :)
<lvh>	aquarius, it would make much more sense to just use the sentinel value, surely
<lvh>	aquarius, the reason you would use type is because the type is always the same object
<lvh>	aquarius, which is a critically important quality of sentinels (but I'm starting to sound like a stuck record here)
<aquarius>	*nod*
<lvh>	aquarius, so basically everything you've come up with yourself is secretly a sentinel already anyway
<aquarius>	I really don't want to have to dictate that every time we define a new Exception that the API can throw we've gotta also define an equivalent NotQuiteAnException thing as well which looks identical. It seems horrid.
<lvh>	aquarius, I think you have too many exceptions
<aquarius>	I'd define one single SomethingHasGoneWrongException and set exception.reason = NotQuiteAnExceptionSubclass if you could do "except" based on attributes of the exception :)
<lvh>	aquarius, but you can't use except, because you're not raising anything
<aquarius>	in put_many_records, sure. But I do in put_one_record, because that's the logical way to do it. If I start returning things and then make everyone check the return value to see if it means there was a problem, I might as well be a C programmer and kill myself. :)
<lvh>	so basically what we actually want is exceptions that don't lose state when travelling up the stack
<lvh>	but python doesn't do it
<lvh>	aquarius, ah, right, I was assuming we're talking about the function that still has to be written
<lvh>	aquarius, there is one way though, I guess, sort of
<aquarius>	we are, mostly, and obviously the way you call put_many_records will differ from the way you call put_one_record, but they should share as much as possible, purely under the principle of least surprise :)
<lvh>	aquarius, raise an exception that contains enough state to pick up where you left off, but I'm not entirely sure what you would want your API to look like then
<aquarius>	if you've already written code that calls put_one_record lots of times and you want to update your code to the new put_many_records API, you should't have to totally throw away all your handling code and redo it because something totally different happens, if at all possible.
<lvh>	aquarius, that would have to be a custom class with a very interesting __iter__
<lvh>	since iter is only called once
<lvh>	aquarius, right
<lvh>	aquarius, be right back, I think this might not be entirely terrible
<lvh>	aquarius, you can't do it without some except SomeError, e: call_the_loop_again(e.enough_state_to_call_the_loop_again)
<lvh>	aquarius, you could do it with exceptions but the exceptions need to contain enough information to regenerate the state of the generator that you just lost because it raised an exception and it wasn't caught inside the generator
<lvh>	aquarius, unfortunately you can't pickle generators
<aquarius>	*nod*
<lvh>	aquarius, but you can store return values in a classattr of a Results class
<lvh>	and clean up on __del__
<aquarius>	yeah, I've wondered about returning a Results object rather than just a plain list.
<lvh>	continueFrom(e) doesn't seem entirely disgusting but you would need to call the for loop recursively, afaik you can't change the thing the for loop is currently iterating over
<lvh>	without disgusting inspection tricks
<lvh>	you can, of course
<lvh>	in C
<lvh>	but I will shoot you in the face if you do it
*	aquarius grins
<aquarius>	no intention of doing that :)
<lvh>	aquarius, never mind the shooting in the face, I'm not even sure if it'd be that bad actually
<lvh>	you're doing something unpythonic in order to do something pythonic
<lvh>	also it involves frame inspection, so maybe this would be a fitting penance for suggesting Exception instances as return values
<lvh>	aquarius, perhaps you should write some of your talking and my blubbering up and pour it into a comp.lang.python post
<aquarius>	that's not a bad idea, actually.
*	aquarius adds to todo list
<lvh>	aquarius, to recap my final idea that I realise is disgusting: C code would figure out through inspection what the iterator is that the for loop that's iterating over the Results object is currently iterating over
<lvh>	aquarius, then replace that PyObject* pointer with one to a newly generated iterator based on the last raised exception, which has a reference to the set of not-yet-processed things
<lvh>	aquarius, either mention that I thought this was ugly or don't mention my name -- I'd like to be employable if my current job goes tits up
<aquarius>	lvh, jesus wept. That's horrid. :)
<lvh>	aquarius, none of the alternatives allow you to use try/except clauses
<lvh>	aquarius, except ones that are worse
<aquarius>	lvh, yeah, I see the reason for it, it's just that I have to be able to maintain this code a year from now and I'm not as clever as you :)
<lvh>	aquarius, this is the bad kind of clever
<lvh>	aquarius, it's the kind of clever that made me realise i need to stop doing perl
*	aquarius laughs
<aquarius>	also, if I write that, which I'm not totally sure I'm capable of, and submit it for code review, my team will boil me alive :)
<lvh>	aquarius, it's not *thhaaaaat* bad
<lvh>	aquarius, the thing you're iterating over is still bound to a name, so  you can just implement Results.__iter__ to store a local copy of the last-returned iterator
<lvh>	aquarius, that way you don't even have to use C or anythin'!
<lvh>	aquarius, well, except for getting around the exceptions that aren't really exceptions bit of course
<aquarius>	lvh, yeah. food for thoughts.
<lvh>	aquarius, s/thoughts/nightmares/

review: Needs Information

Revision history for this message

Manuel de la Peña (mandel) wrote on 2010-01-18:

#

Download full text (21.6 KiB)

I WOW that is indeed a long conversation (an instructive too), but there is a concern I have, if we were to fix this issue regarding the returning of exceptions we should first patch the update method of python-couchdb since it is the one really performing the job, the put_records_batch simple uses it and later adds the required attachments. From my point of view it is not worth to look at a solution at desktpcouch level but on python-couchdb.

I'm sure that the guy at python-couchdb will understand that returning instances of exceptions is not the best way, but will they want to depend on twisted? I personally dont think so, nevertheless we will have to talk with them and fix the real code that has the problem, if exceptions are created by python-couchdb we are simply making things worse.

I hope I make sense,

Manuel

> Man, I don't like returning an exception instance. After a long discussion on
> #python, though, I'm not sure there's a better way. Entire discussion pasted
> below (sorry, it's quite long).
>
> <aquarius> If I've got a function which does multiple things, some of
> which may fail, but failure of some things doesn't mean that the others didn't
> succeed, what's the best way of signalling that failure? If I raise a
> ItPartiallyWorked exception, then I'll still need to pass back the successful
> results (from the successful actions) in that exception too, which seems a bit
> wrong
> <lvh> aquarius, a warning sounds in order
> <lvh> aquarius, just return a tuple that might have None in some bits
> <lvh> also, the "warning" module
> <aquarius> lvh, well, that's what we're doing at the moment: we return a
> list of results, where each result is either a successful result or an
> exception. But returning a list with exception instances in it seems weird. I
> mean, we could throw a warning *too* to alert people that it happened, of
> course.
> <lvh> aquarius, you probably want to split the separate bits up into smaller
> functions that *do* do the right thing and raise an exception, but aren't
> taxed with the extra problem of still delivering partial results, like the
> composing function
> <Alberth> aquarius: output a warning line to stderr ?
> <aquarius> lvh, ah, I can't split it up into little bits because I'm
> calling an external non-Python thing which actually does all the actions :)
> <lvh> aquarius, yeah no, don't use exception objects as sentinel values
> <lvh> aquarius, use None instead, your users will hate you less
> <aquarius> lvh, so I pass all the actions in one go to the external thing
> (CouchDB, in this case), and it gives me back a list of successes or failures.
> <lvh> aquarius, does the absolute order matter? IE is the fact that
> something is at position 2 meaningful?
> <aquarius> if I use None, though, there's no indication of *why* that
> action failed.
> <lvh> aquarius, or is it just the relative position (2 comes before 3 and
> after 1)
> <aquarius> lvh, absolute order does not matter (the list that comes back
> is *actually* a list of tuples (id_of_action, success_or_failure)
> <Alberth> aquarius, lvh: you can also use a dict {'stepA': 'ok',
> '...

I WOW that is indeed a long conversation (an instructive too), but there is a concern I have, if we were to fix this issue regarding the returning of exceptions we should first patch the update method of python-couchdb since it is the one really performing the job, the put_records_batch simple uses it and later adds the required attachments. From my point of view it is not worth to look at a solution at desktpcouch level but on python-couchdb.

I'm sure that the guy at python-couchdb will understand that returning instances of exceptions is not the best way, but will they want to depend on twisted? I personally dont think so, nevertheless we will have to talk with them and fix the real code that has the problem, if exceptions are created by python-couchdb we are simply making things worse.

I hope I make sense,

Manuel

> Man, I don't like returning an exception instance. After a long discussion on
> #python, though, I'm not sure there's a better way. Entire discussion pasted
> below (sorry, it's quite long).
> 
> <aquarius>      If I've got a function which does multiple things, some of
> which may fail, but failure of some things doesn't mean that the others didn't
> succeed, what's the best way of signalling that failure? If I raise a
> ItPartiallyWorked exception, then I'll still need to pass back the successful
> results (from the successful actions) in that exception too, which seems a bit
> wrong
> <lvh>   aquarius, a warning sounds in order
> <lvh>   aquarius, just return a tuple that might have None in some bits
> <lvh>   also, the "warning" module
> <aquarius>      lvh, well, that's what we're doing at the moment: we return a
> list of results, where each result is either a successful result or an
> exception. But returning a list with exception instances in it seems weird. I
> mean, we could throw a warning *too* to alert people that it happened, of
> course.
> <lvh>   aquarius, you probably want to split the separate bits up into smaller
> functions that *do* do the right thing and raise an exception, but aren't
> taxed with the extra problem of still delivering partial results, like the
> composing function
> <Alberth>       aquarius: output a warning line to stderr ?
> <aquarius>      lvh, ah, I can't split it up into little bits because I'm
> calling an external non-Python thing which actually does all the actions :)
> <lvh>   aquarius, yeah no, don't use exception objects as sentinel values
> <lvh>   aquarius, use None instead, your users will hate you less
> <aquarius>      lvh, so I pass all the actions in one go to the external thing
> (CouchDB, in this case), and it gives me back a list of successes or failures.
> <lvh>   aquarius, does the absolute order matter? IE is the fact that
> something is at position 2 meaningful?
> <aquarius>      if I use None, though, there's no indication of *why* that
> action failed.
> <lvh>   aquarius, or is it just the relative position (2 comes before 3 and
> after 1)
> <aquarius>      lvh, absolute order does not matter (the list that comes back
> is *actually* a list of tuples (id_of_action, success_or_failure)
> <Alberth>       aquarius, lvh: you can also use a dict {'stepA': 'ok',
> 'stepB': 'foo failed'}
> <lvh>   if you raise an exception inside a generator and you only catch it
> outside of it, is the state lost?
> <lvh>   the logical thing to assume would be yes, let me give it a shot
> <aquarius>      Alberth, yes. that's what we do. What should "foo failed" be,
> though? I mean, it could just be a string, but then what I've done is
> reinvented string Exceptions :)
> <lvh>   aquarius, sentinel value
> <lvh>   aquarius, typically yourmodule.FAILURE or None
> <lvh>   of course, FAILURE = None # at module scope works fine
> <aquarius>      lvh, a sentinel value won't tell me what type of failure it
> was, though, it'll just tell me that it did fail. I'd like to know why
> <Alberth>       nice idea, a set of constant values could work
> <Alberth>       NO_FOO=1  ; NO_BAR =2
> <aquarius>      but that's just exceptions reinvented!
> <lvh>   aquarius, no, return values do not travel up the stack
> <Alberth>       exceptions are used to stop execution imho
> <lvh>   well, they do: but only once
> <Alberth>       that is not what you do here, here you want to continue, and
> return a collection of results upwards
> <lvh>   right
> <aquarius>      so if there are fifty separate ways it can fail, and for the
> "do one action" function I've defined fifty different exceptions, I now need
> to define fifty equivalent sentinel values as well so they can be returned by
> the multiple-actions function? this seems...less than efficient.
> <lvh>   aquarius, if you have fifty different ways it can fail it shouldn't be
> one giant black box
> <lvh>   aquarius, chopping it up into tiny bits was the first thing I
> suggested for a reason ;-)
> <aquarius>      lvh, why? urllib.urlopen can fail in fifty different ways for
> exactly the same reason; the remote web server can throw loads of different
> HTTP errors, the network might not be there, etc.
> <lvh>   aquarius, and only a few of those are meaningful to catch
> <aquarius>      lvh, I understand the point about chopping it up, and you are
> welcome to call do_one_action twenty times and trap the exception, but the
> whole point of the multiple-actions thing is precisely that you can call it
> once with twenty things, it calls the external process (CouchDB) once with all
> twenty things, and it'll tell you whether each succeeded or failed. It's for
> efficiency.
> <lvh>   aquarius, and for each of the twenty things twenty *distinct* problems
> can happen?
> <aquarius>      lvh, could do, yes.
> <lvh>   aquarius, then I don't understand how your function works at all
> <lvh>   aquarius, if you're making an n-ary version of do_one_action i would
> expect the n-ary to not return significantly different exceptions
> <lvh>   besides perhaps TypeError (the thing you gave me isn't iterable)
> <aquarius>      lvh, CouchDB is a database whose API is HTTP. So to store a
> new document in it you PUT/POST the document contents to a URL specific to
> that document. A document PUT may succeed, or it may fail because it conflicts
> with an existing document or because it violates the rules about what a
> document may contain in many different ways, or because the server is down.
> <lvh>   aquarius, I know CouchDB
> <aquarius>      lvh, ah, ok. so do_many_actions wraps Couch's bulk update
> function.
> <lvh>   aquarius, right -- but none of those errors are *distinct*
> <lvh>   aquarius, all of the failures that happen on the n-ary can also happen
> on a single update, right?
> <aquarius>      correct.
> <lvh>   aquarius, now it's making much more sense
> <aquarius>      but they can happen individually to each component of an
> update, and they may happen to some components and not others
> <aquarius>      we use validation, so that a document must conform to a
> specific schema (for instance, a "contact" document must have a name
> <lvh>   aquarius, twisted had to solve a similar problem with Deferreds and
> Failures
> <aquarius>      lvh, violating that validation throws an error specific to the
> violation ("a contact record must have a name field"), for example
> <lvh>   aquarius, sentinel values is how you do it, sorry
> <aquarius>      lvh, so I don't want to have to define a sentinel value for
> every possible failure, since there are *loads*, and we already have
> exceptions for this
> <lvh>   well, twisted didn't really do it with a sentinel value, but
> twisted.python.failure.Failure isn't really what you need
> <aquarius>      what's actually the problem with passing back an exception
> instance, other than inelegance?
> <lvh>   aquarius, because then you're using instances as sentinel values
> <lvh>   aquarius, the point of sentinel values is that everyone uses the exact
> same object
> <lvh>   which is why None is so popular: it's a guaranteed singleton
> <aquarius>      yeah, but...so? I don't really care whether I can deduce that
> row5 and row12 failed for the same reason. I never need to test equivalence
> between sentinels, which is why I don't think that sentinels are the right way
> :)
> <Alberth>       if you don't need equivalence, why not use a boolean?
> <lvh>   aquarius, exceptions travel up the stack until caught
> <aquarius>      Alberth, because a boolean won't tell me *why* it failed, just
> that it did
> <aquarius>      lvh, only if they're raised.
> <lvh>   traveling up the stack is not what you want, because you store state
> at the bottom of it
> <lvh>   aquarius, I've already told you why the only thing you do with
> exceptions is raise
> <Alberth>       aquarius: how are you deciding that without comparing them?
> <lvh>   aquarius, how would *returning* an exception instance be different
> from using a sentinel value
> <lvh>   besides being more useless because sentinel values are typically used
> because you know everyone's using the same one
> <aquarius>      Alberth, I can compare exception_instance.reason, which is a
> string, if I want to; that's effectively a sentinel.
> <lvh>   aquarius, sure, but why does it need to be an exception to do that?
> <Alberth>       pass the string up instead?
> <lvh>   Alberth, I'm not sure I like that, strings are a data structure of
> last resort
> <Alberth>       NO_FOO="no foo" ; NO_BAR="no bar"
> <aquarius>      Alberth, yep, that's what I thought of, but passing back a
> string instead of a more complex data structure means that it's hard to pass
> additional data. This is (one of the reasons) why string exceptions went away.
> <lvh>   aquarius, I don't understand why you want it to be an Exeption rather
> than any other type
> <lvh>   *Exception
> <lvh>   aquarius, I understand your argument for complex data structures and I
> agree
> <lvh>   aquarius, twisted needed that and that resulted in
> twisted.python.failure.Failure
> <aquarius>      lvh, it doesn't have to be an Exception. It just seemed a bit
> silly to me to create some other type MySignallingOfAnErrorThing in fifty
> different varieties when I'll use them in a very similar way to Exceptions,
> and when I already have Exceptions defined for all these things
> <aquarius>      seems...wasteful.
> <lvh>   aquarius, <aquarius> yeah, but...so? I don't really care whether I can
> deduce that row5 and row12 failed for the same reason. I never need to test
> equivalence between sentinels, which is why I don't think that sentinels are
> the right way :) # could you explain that in more detail? I'm not sure I
> understand
> <lvh>   also, I don't agree with "when I'll use them in a very similar way to
> Exceptions"
> <lvh>   you're using them in a way that isn't like an exception at all
> <Alberth>       both lvh and I believe that an exception object should be used
> only for raise/catch, and not as data value.
> <lvh>   actually, come to think of it, if you write your thing as a generator
> twisted.python.failure.Failure.throwExceptionIntoGenerator might be *exactly*
> what you want
> <aquarius>      imagine that, say, if PUTting a new record to Couch results in
> a NoNameDefined problem. My put_one_record function, if Couch throws that
> problem, would raise a NoNameDefined exception and I want to tell the punter
> that. So calling it would be "try: put_one_record(data); except NoNameDefined:
> error_list.add("You didn't define a name")
> <lvh>   aquarius, presumably append and not add, but I get what you're saying
> <lvh>   aquarius, basically you want to be able to preserve generator state
> despite uncaught exceptions
> <aquarius>      the put_many_records would do something very similar:
> "results=put_many_records(list_of_data); for result in results: if
> result.problem == NoNameDefinedSentinelValue: error_list.add("You didn't
> define a name for %s" % result.id)
> <lvh>   aquarius, but I'm pretty sure you can't do that without ugly hacks or
> a time machine
> <lvh>   or twisted.python.failure.Failure.throwExceptionIntoGenerator
> <aquarius>      those two bits of code are way similar, it's just that one
> uses exceptions and the other doesn't. that's what I mean by "using them
> similarly"
> <aquarius>      (I used "add" 'cos I'm thinking of error_list as a Gtk list
> display widget or something, but that's not important :))
> <aquarius>      yeah. I couldn't really think of a decent way to do this; it's
> all a bit hacky, hence coming here in case there was something cool I don't
> know about :)
> <lvh>   aquarius, twisted.python.failure.Failure.throwExceptionIntoGenerator
> is something cool and you probably didn't know about it
> <aquarius>      the twisted thing might be good, but...is it independent, or
> do I have to turn my app into a Twisted app to get at it?
> <aquarius>      lvh, already reading about it ;)
> <lvh>   aquarius, pretty much everything in twisted.python can be used in 3rd
> party non-twisted apps
> <lvh>   BUT Failure is explicitly designed for asynchronous error handling
> because exceptions aren't good at it
> <lvh>   aquarius, in all honesty you're probably better off doing what you're
> doing now, except don't make the things you're returning Exceptions
> <lvh>   either use sentinel values, or, if you need to pass more data, custom
> types
> <lvh>   if you don't really care *which* row failed, use custom types
> <lvh>   errr, sentinel values
> <aquarius>      lvh, that's the conclusion I'm coming to...you still think
> that I should define fifty different
> ThingThatIsSortaLikeAnExceptionButNotReallys, one for each Exception I've got?
> <lvh>   aquarius, I think you probably shouldn't have fifty
> <lvh>   aquarius, and I think you probably shouldn't have the n-ary thing in
> your API
> <lvh>   especially if it makes error handling this difficult
> <lvh>   why don't people just do a for x in L: try: do_one_thing(x); except
> UglyError: continue?
> <aquarius>      lvh, it's in couch's API for a reason, though, and that's that
> making one HTTP call is way, way more efficient if you're trying to update 200
> documents at once.
> <aquarius>      let alone 20000 :-)
> <lvh>   aquarius, if you're convinced you need that, sentinel values or frame
> inspection
> <aquarius>      at the moment you precisely do have to call do_one_thing 20000
> times; we're wrapping couch's bulk_update API now, which is why we're trying
> to work out how to do it in a way that doesn't suck :)
> <aquarius>      frame inspection?
> <lvh>   aquarius, you should probably forget I suggested that
> <aquarius>      every time I've manually crawled up the stack and looked at
> frames I've felt guilty about it and six months later I haven't been able to
> understand why I did it, but that doesn't mean that there's not some case
> somewhere where it's actually a good idea ;)
> <lvh>   aquarius, anyway, the reaosn i didn't want exception instances is
> because you need to be able to figure out WHICH ONE it is at runtime
> <lvh>   which is why identiy in sentinel values is a good idea
> <aquarius>      WHICH ONE in what sense? I mean, I can tell what type of
> exception it is, no?
> <lvh>   aquarius, how? typechecking?
> <lvh>   a dispatch dict is way cleaner
> <lvh>   or a sequence of "if e is mymodule.QUANTUM_TRANSMOGRIFIER_EXPLOSION:"
> <aquarius>      lvh, I can have a dispatch dict keyed with Exception
> subclasses, though, no? and then dispatch on isinstance
> <Alberth>       aquarius: instead of trying one big do-everything-in-one-call,
> why not split it in phases?  make an object that collects sub-queries you have
> to do (as sub-data objects), a single 'do-it' that takes the collected sub-
> queries, and does them together, and stores the exec results back in the sub-
> queries in the sub-data objects, then investigate each sub-query object for
> the result afterwards?
> <lvh>   aquarius, no, you can't dispatch inside a dictionary based on a
> boolean value
> <lvh>   aquarius, you can do it with type instead of isinstance, but then
> you're using the type in order to get to a sentinel value
> <aquarius>      lvh, yeah, I just realised that my idea's stupid :)
> <lvh>   aquarius, it would make much more sense to just use the sentinel
> value, surely
> <lvh>   aquarius, the reason you would use type is because the type is always
> the same object
> <lvh>   aquarius, which is a critically important quality of sentinels (but
> I'm starting to sound like a stuck record here)
> <aquarius>      *nod*
> <lvh>   aquarius, so basically everything you've come up with yourself is
> secretly a sentinel already anyway
> <aquarius>      I really don't want to have to dictate that every time we
> define a new Exception that the API can throw we've gotta also define an
> equivalent NotQuiteAnException thing as well which looks identical. It seems
> horrid.
> <lvh>   aquarius, I think you have too many exceptions
> <aquarius>      I'd define one single SomethingHasGoneWrongException and set
> exception.reason = NotQuiteAnExceptionSubclass if you could do "except" based
> on attributes of the exception :)
> <lvh>   aquarius, but you can't use except, because you're not raising
> anything
> <aquarius>      in put_many_records, sure. But I do in put_one_record, because
> that's the logical way to do it. If I start returning things and then make
> everyone check the return value to see if it means there was a problem, I
> might as well be a C programmer and kill myself. :)
> <lvh>   so basically what we actually want is exceptions that don't lose state
> when travelling up the stack
> <lvh>   but python doesn't do it
> <lvh>   aquarius, ah, right, I was assuming we're talking about the function
> that still has to be written
> <lvh>   aquarius, there is one way though, I guess, sort of
> <aquarius>      we are, mostly, and obviously the way you call
> put_many_records will differ from the way you call put_one_record, but they
> should share as much as possible, purely under the principle of least surprise
> :)
> <lvh>   aquarius, raise an exception that contains enough state to pick up
> where you left off, but I'm not entirely sure what you would want your API to
> look like then
> <aquarius>      if you've already written code that calls put_one_record lots
> of times and you want to update your code to the new put_many_records API, you
> should't have to totally throw away all your handling code and redo it because
> something totally different happens, if at all possible.
> <lvh>   aquarius, that would have to be a custom class with a very interesting
> __iter__
> <lvh>   since iter is only called once
> <lvh>   aquarius, right
> <lvh>   aquarius, be right back, I think this might not be entirely terrible
> <lvh>   aquarius, you can't do it without some except SomeError, e:
> call_the_loop_again(e.enough_state_to_call_the_loop_again)
> <lvh>   aquarius, you could do it with exceptions but the exceptions need to
> contain enough information to regenerate the state of the generator that you
> just lost because it raised an exception and it wasn't caught inside the
> generator
> <lvh>   aquarius, unfortunately you can't pickle generators
> <aquarius>      *nod*
> <lvh>   aquarius, but you can store return values in a classattr of a Results
> class
> <lvh>   and clean up on __del__
> <aquarius>      yeah, I've wondered about returning a Results object rather
> than just a plain list.
> <lvh>   continueFrom(e) doesn't seem entirely disgusting but you would need to
> call the for loop recursively, afaik you can't change the thing the for loop
> is currently iterating over
> <lvh>   without disgusting inspection tricks
> <lvh>   you can, of course
> <lvh>   in C
> <lvh>   but I will shoot you in the face if you do it
> *       aquarius grins
> <aquarius>      no intention of doing that :)
> <lvh>   aquarius, never mind the shooting in the face, I'm not even sure if
> it'd be that bad actually
> <lvh>   you're doing something unpythonic in order to do something pythonic
> <lvh>   also it involves frame inspection, so maybe this would be a fitting
> penance for suggesting Exception instances as return values
> <lvh>   aquarius, perhaps you should write some of your talking and my
> blubbering up and pour it into a comp.lang.python post
> <aquarius>      that's not a bad idea, actually.
> *       aquarius adds to todo list
> <lvh>   aquarius, to recap my final idea that I realise is disgusting: C code
> would figure out through inspection what the iterator is that the for loop
> that's iterating over the Results object is currently iterating over
> <lvh>   aquarius, then replace that PyObject* pointer with one to a newly
> generated iterator based on the last raised exception, which has a reference
> to the set of not-yet-processed things
> <lvh>   aquarius, either mention that I thought this was ugly or don't mention
> my name -- I'd like to be employable if my current job goes tits up
> <aquarius>      lvh, jesus wept. That's horrid. :)
> <lvh>   aquarius, none of the alternatives allow you to use try/except clauses
> <lvh>   aquarius, except ones that are worse
> <aquarius>      lvh, yeah, I see the reason for it, it's just that I have to
> be able to maintain this code a year from now and I'm not as clever as you :)
> <lvh>   aquarius, this is the bad kind of clever
> <lvh>   aquarius, it's the kind of clever that made me realise i need to stop
> doing perl
> *       aquarius laughs
> <aquarius>      also, if I write that, which I'm not totally sure I'm capable
> of, and submit it for code review, my team will boil me alive :)
> <lvh>   aquarius, it's not *thhaaaaat* bad
> <lvh>   aquarius, the thing you're iterating over is still bound to a name, so
> you can just implement Results.__iter__ to store a local copy of the last-
> returned iterator
> <lvh>   aquarius, that way you don't even have to use C or anythin'!
> <lvh>   aquarius, well, except for getting around the exceptions that aren't
> really exceptions bit of course
> <aquarius>      lvh, yeah. food for thoughts.
> <lvh>   aquarius, s/thoughts/nightmares/

Revision history for this message

Manuel de la Peña (mandel) wrote on 2010-01-25:

#

I have been doing my little inquiries in python-couchdb, please read the following conversations regarding the matter:

Original discussion on the update interface: http://groups.google.com/group/couchdb-python/browse_thread/thread/df58036112c90040/e49d948a152c839b?lnk=gst&q=deferredlist#e49d948a152c839bfor

My conversation with python-couchdb group: http://groups.google.com/group/couchdb-python/browse_thread/thread/bf51ec2cd24ad154

I don't see any practical implementation of the method that does not return instances of different exceptions.

Revision history for this message

Stuart Langridge (sil) wrote on 2010-01-25:

#

OK. There seems to be no very good way of handling this, so we'll go with what you currently have.

review: Approve

Revision history for this message

John O'Brien (jdobrien) wrote on 2010-01-25:

#

looks good.

review: Approve

Revision history for this message

Chad Miller (cmiller) wrote on 2010-02-25:

#

Attempt to merge lp:~mandel/desktopcouch/batch_update into lp:desktopcouch failed due to merge conflicts:

text conflict in desktopcouch/records/server_base.py

Revision history for this message

Manuel de la Peña (mandel) wrote on 2010-02-25:

#

Should be ok now.

> Attempt to merge lp:~mandel/desktopcouch/batch_update into lp:desktopcouch
> failed due to merge conflicts:
>
> text conflict in desktopcouch/records/server_base.py

lp:~mandel/desktopcouch/batch_update updated on 2010-02-25

120. By Manuel de la Peña on 2010-02-25: Merged with current trunk

 === modified file 'desktopcouch/records/server_base.py'
 --- desktopcouch/records/server_base.py	2010-02-25 19:59:13 +0000
 +++ desktopcouch/records/server_base.py	2010-02-25 21:18:17 +0000
@@ -215,6 +215,32 @@
          return record.record_id
++    def put_records_batch(self, batch):
++        """Put a batch of records in back end storage."""
++        # used to access fast the record
++        records_hash = {}
++        for record in batch:
++            if not record.record_id:
++                from uuid import uuid4  # Do not rely on couchdb to create an ID for us.
++                record.record_id = uuid4().hex
++            records_hash[record.record_id] = record
++        # although with a single record we need to test for the revisison, with
++        # a batch we do not, but we have to make sure that we did not get an error
++        batch_put_result = self.db.update(batch)
++        for current_tuple in batch_put_result:
++            success, docid, rev_or_exc = current_tuple
++            if success:
++                record = records_hash[docid]
++                # set the new rev
++                record._data["_rev"] = rev_or_exc
++                for attachment_name in record.list_attachments():
++                    data, content_type = record.attachment_data(attachment_name)
++                    self.db.put_attachment(
++                        {"_id":record.record_id, "_rev":record["_rev"]},
++                        data, attachment_name, content_type)
++        # all success record have the blobs added we return result of update
++        return batch_put_result
++
      def update_fields(self, record_id, fields, cached_record=None):
          """Safely update a number of fields. 'fields' being a
          dictionary with path_tuple: new_value for only the fields we
 === modified file 'desktopcouch/records/tests/test_server.py'
 --- desktopcouch/records/tests/test_server.py	2010-02-13 22:06:45 +0000
 +++ desktopcouch/records/tests/test_server.py	2010-02-25 21:18:17 +0000
@@ -95,6 +95,26 @@
          self.assertEqual(
              record['record_number'], retrieved_record['record_number'])
++    def test_put_records_batch(self):
++        """Test putting a batch of records."""
++        # create records to be put in a batch
++        batch = []
++        for current in (1, 2, 3, 4, 5):
++            record = Record({'record_number': current}, record_type="http://example.com/")
++            if current % 2:
++                # set bad data to get an exception
++                record._data["_rev"] = "1-32323"
++            batch.append(record)
++        # put the batch and ensure that the records have been added
++        batch_result = self.database.put_records_batch(batch)
++        for current_tuple in batch_result:
++            success, docid, rev_or_exc = current_tuple
++            if success:
++                self.assertTrue(self.database._server[self.dbname][docid])
++            else:
++                # make sure we do not have the record in the db
++                self.assertFalse(self.database._server[self.dbname][docid])
++
      def test_delete_record(self):
          """Test deletion of records."""
          record = Record({'record_number': 0}, record_type="http://example.com/")

desktopcouch

Merge lp:~mandel/desktopcouch/batch_update into lp:desktopcouch

Commit message

Description of the change

Preview Diff

Subscribers