Brock Pytlik wrote: > Tom Mueller (pkg-discuss) wrote: >> Brock, >> >> Please consider these comments of a more general nature (looks like >> Shawn got the detailed ones). >> >> Please change the API to be more object oriented: >> - The term "PkgClient" doesn't represent what the thing is that is >> being manipulated here. The object here is an Image. If a client is >> going to manipulate multiple images at the same time, it would need >> multiple of these objects, but there is still only one client. >> > I agree. I'm open to better suggestions. I would suggest Image. >> - The PkgClient.describe method returns a PlanDescription, so it >> would seem that the thing being described in a "Plan" but there isn't >> any "Plan" object in the API. >> > Why would there be a plan object in the API that the outside world > should know about? You tell the API to make a plan. If you want to, > you can ask for a description of what that plan will do (which gives > the PlanDescription). Hiding the plan is one way we're refactoring the > interfaces to present a consistent and simple interface to the client. > If you can present an example where the client needs to modify the > plan, I'd be very interested to hear it. A typical operation for a GUI is:
- make a plan - get some information from the plan and query the user, e.g., -- confirm that the dependencies should be installed too? -- confirm the license? - execute the plan If the plan is a first class object in the interface, then the first step is an operation on the Image that returns a Plan, and then the remaining steps are operations on the Plan. In the internal API, these are separated into the Image object and the ImagePlan object. Merging all of this into a single class doesn't necessarily make things consistent and simple - it just makes a single class with more methods. >> I have yet to see a Python library that has a module called "api". >> Can you provide an example of another Python library that uses this >> pattern? As a counter-example, consider the CherryPy and PyOpenSSL >> libraries that we use. CherryPy exposes the cherrypy and >> cherrypy.wsgiserver modules as public interfaces. PyOpenSSL exposes >> the OpenSSL and OpenSSL.crypto modules. Even for a large Python >> system like mercurial, there is no "api" module. One just uses "from >> mercurial import hg" when writing an extension to mercurial. Please >> rename the "api" module to represent what interface is actually being >> used. I'd suggest "pkg.client.image", but that is already taken. >> ;-) Maybe these classes could be moved into pkg/client/__init__.py >> in such a way that a programmer would just do "import pkg.client". >> > Is this an objection to the naming of "api.py"? yes. > If so, I'd happily take suggestions. I'm mildly against moving things > into __init__.py as I think it's a fairly python specific construction > which provides a needless barrier to entry to others coming to use our > code, something I'd suggest is something an API should avoid where > possible. Moving it into __init__.py makes it so that the only interface the users need to know about is "import pkg.client". Seems pretty simple. I don't know why this would be a barrier to entry, assuming that there is appropriate documentation with the API. > > Also, the argument that others don't do this isn't, by itself > tremendously convincing to me. Certainly, I'll take another look at > how those programs are structured, but because CherryPy or PyOpenSSL > do things one way doesn't mean it's the right way for us to do it. When I do something that is different from every other example I see, it's either because I've come up with something no one else has thought of, or it's because that thing has already been rejected by everyone else. For me, I usually assume it is the latter. >> There is insufficient encapsulation in the API. - the PackageInfo >> object exposes version.DotSequence objects. Is that intentional? >> i.e., is version.DotSequence part of the external API? >> > The decision about how to expose Fmris, Versions, and Dot sequencies > hasn't been made yet. Mostly because I don't have a strong feeling yet > about what the right answer is. As answer to this and many of your > subsequent questions: this is an API in developement, really, an > intermediate API. Things will change. Things that are exposed may > become hidden, and things that are hidden may become exposed as I > discover information insufficiencies in the API. (For example, > refresh currently returns an actual image. It does this because, given > the other functions implemented, there would be no way for the GUI to > update it's information after a refresh without rereading from disk, > something that seemed silly to do since that information was already > in memory. So, for now, it returns the image, a clear violation of the > spirit of the API, and something that will be changed once it's more > fully functional.) I'm confused then. CR 3443 says that the purpose is to provide a "stable" API. But yet this is an intermediate API that is going to change. I understand that this has to be evolutionary, but it seems that there has to be a certain amount of stability before this is first used. My reason for bringing up these issues is that it doesn't seem this minimal level of stability is there yet. >> - the PkgClient class requires a ProgressTracker object. So is this >> API making ProgressTracker a public interface? Also, if one wants to >> use different progress trackers for different operations, how does >> one do that? (for example, different progress trackers for loading >> the catalogs vs. making the install plan in the plan_install method.) >> > Yes, as I've said previously in email here, the ProgressTracker is > part of the API. Is there a document that lists what classes and methods are all part of the API, similar to what we see in the Python manual for each module? > > Can you present a use case for A) Why you'd want to switch progress > trackers partway through an operation B) Why you couldn't just make > your own hybrid of the progress tracker which does what you want it to? The updatetool GUI uses a QuietProgressTracker to load the catalogs and then uses a custom-defined progress tracker that implements a progress bar dialog for evaluating and executing the install plan. One could certainly implement a hybrid - that seems like a reasonable option. >> - how many other internal classes are exposed through this API >> > I have no idea right now. Probably a lot given that this is still a > work in process. >> Why does the API have "__" methods (like __licenses). Shouldn't this >> functionality be in the backend? Isn't the problem with the existing >> API that there are too many private methods? >> > No, the problem with the existing API is not that there are too many > private methods. But if you'd be happier, I can probably move > __licenses into image.py. It ended up there probably because, > originally, licenses was an API function, which became a private > method when I reorganized some other things. Despite that, yes, the > API will have private methods. If clients call private methods they're > unsupported, just like if function in image.py decided to call > _write_main_dict_line in indexer.py directly, they'd be unsupported. >> All exceptions should return data that can be extracted by the >> client. VersionException and several others are good examples of >> this. But ProblematicPermissionsIndexException doesn't do this. >> > What data would you like it to return? When I first read this, I didn't realize the "cause" is already available. So the data is the cause (with a clarification of what this is - it appears to be a string that is a pathname). There needs to be documentation for each exception saying what data is available with the exception. In Java, this would be done with accessor methods and private data. Since accessor methods and private data is not generally the norm in Python, this has to be done with documentation. Otherwise, how does the user of the exception know what data is available? >> What is the intent of the __activity_lock functionality? We >> eventually need to have locking that prevents multiple clients (in >> separate processes) from modifying an image concurrently. Once we >> have that, the same functionality will also work for different >> threads in a single process. So thread-based locking is not really >> useful here. It's also not clear to me that this API layer should be >> responsible for locking, as that may not provide sufficient granularity. >> > The point of locking is so that, for example, multiple GUI threads > can't start planning an install, an uninstall, and an image_update all > at the same time on the same PkgClient object, for example. Can this > become more finely grained in the future? Probably. But for now, they > help make the code thread safe, something that's important for the > GUI. When we have the more general mechanism, perhaps, and only > perhaps, it can replace the mechanisms here. IMHO, cross process concurrency is a bigger issue. And solving that problem would solve the thread one too. >> At what point would you expect the imports of the other pkg.client.* >> and pkg.* modules to be removed from client.py? > When the API is complete, and not in development. >> (before or after the first checkin of this API?) > After, since I foresee at least 2 stages of commits, and probably 3 >> I'd like to at least see a plan for how complicated this api module >> will get in order to be able to remove those imports, so that the >> true expense of this option will be understood before the team has to >> live with it. >> > So, what you'd like is for me to do all the work needed to completely > analyze what the final API would look like (which basically means > creating and implementing the final API since, at least for me, that's > the only way to be certain I've gotten things correct). That's fine, > but there's little chance that can be finished in time for the GUI > team to move to it for the november release. Until I receive firm > direction that we've changed our schedule on this, I'm going to > proceed as I've planned. > > I'll make one other point, if this option, once it's fully in place, > is truly so terrible that we as a team "can't live with it", then we > can blow it up and someone else can try again. All that we'll have > lost is the time I've spent on it (and the amount of time we've spent > kvetching about it). In a previous message[1], Stephen outlined three general options for introducing a stable API. It seems like you've picked option 1 without even considering option 2 or 3. In fact, you've started to write the code for option 1 without having looked at what a design for options 2 or 3 would even look like (at least I haven't seen such a design sent to the pkg-discuss alias, maybe options 2 and 3 were discarded having never made it that far). Since this started, I've just been asking that there be a reasonable evaluation of these various options. This isn't a question of trying one thing and then having someone else try another thing. If the various major design options are considered and then compared side by side, then an appropriate design can be selected. And if option 1 is selected, I can support that (as I've tried to do by providing these comments). Tom [1] http://mail.opensolaris.org/pipermail/pkg-discuss/2008-September/006464.html >> Thanks. >> Tom >> >> >> >> Brock Pytlik wrote: >> >>> Here's the link: >>> http://cr.opensolaris.org/~bpytlik/ips-api-v1/ >>> >>> I haven't done as much cleanup as I usually try to do, but since I'm >>> ooto tomorrow, I thought it would be better to get this out now, >>> rather than next Monday. >>> >>> Other things that I know need to be improved, but that I think can >>> wait for another round of revisions: >>> API Exception organization >>> More complete functionality >>> Automated testing of cancellation (I hope to work with the GUI team >>> to test this by hand in the interim) >>> Versioning of the Progress tracker (perhaps) >>> More Documentation >>> Figure out the future approach to testing we'll take >>> >>> Thanks, >>> Brock >>> _______________________________________________ >>> pkg-discuss mailing list >>> [email protected] >>> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss >>> >> >> _______________________________________________ >> pkg-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss >> > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
