-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
unix/moduselect: poll.unregister: Add optional throw=True param. #4217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
If 2nd, positional only argument of False is passed to poll.unregister() method, then it won't throw exception if passed a stream object not registered with the poller, which was the previous behavior. Then default behavior is made compatible with CPython's, which is to throw exception in this case.
…oller. After previous change, poll.unregister() is now by default compliant with CPython behavior.
This is a follow-up to #4197, implementing handling as proposed in #1550 (comment) |
Making it raise a KeyError if the object is not found, to align with CPython behaviour, makes sense. But it's not clear to me that unregister() needs to be extended to have the proposed "throw" argument. Divergence from CPython should be minimised where possible, and in this case it adds code to diverge. And uasyncio doesn't seem to need this behaviour since it already tracks what is in the poller with objmap. |
It adds idempotent behavior, which is always a useful feature.
Well, everybody knows that, that's why I always followed "social contract" that we try to use existing Python features as far as possible, and if something missing, then extension is adding in a natural, generalizing way. Compare that again with your proposal to add adhoc functions vs my idea to stick with and reuse the standard Python formatting: Here, poll.register() provides useful idempotent behavior, and poll.unregister() accidental-gaply don't. So, we fix it in compatible, natural way.
That's workaround which is supposed (and on the way) to go, per #1550 which talks about adding "callback" data to .register() call, to avoid double O(n) operations (where access to MicroPython dictionaries is O(n)). I'd appreciate if this patch was merged soon, I'm using uasyncio in production, and now I get occasional crashes due to the changed behavior of .unregister(), so I intend to release new version of uasyncio using .unregister(x, False) soon. |
It's still not clear it needs fixing at all. CPython seems fine without such a fix. uasyncio's use can be changed to something like this: if objmap.pop(item, None) is None:
poller.unregister(item) And if that's not enough it can always be wrapped in a try/except block. Why add something that's not necessary and for which there is only one use case so for? |
It's a weird argument to bring in. Turned out, that CPython is not fine without As I mentioned,
What do you mean "one usecase"? This usecase is idempotent operation. Anyone can write an I/O scheduler, and there's high chance that such a feature will be useful, and there will be "many usecases". I don't know why everyone doesn't do that, but I indeed writing an I/O&Task scheduler which strives to be efficient, and number of changes to support that went into MicroPython (and only more in queue). That's "one usecase" which is supposed to enabled many-many usecases (application types which can be developed). On top of that, I mentioned multiple times that I would like to finalize many tasks started, and this is one in the uselect module. The motivation is again: provide user with a freedom of choice of idempotent vs non-idempotent operation. In CPython, poll.register() is idempotent, poll.modify() is kinda non-idemponent variant, but poll.unregister is accidentally only non-idemponent. So, to close the gap, idemponent option is also provided, and there's usecase for it. And there's no need to wait until that usecase hits hard, because then it will be preempted by much more substantial changes and cause unneeded havoc instead of sustainable development. |
I'm sorry, I don't agree with the above arguments. It's a functional addition that is not necessary. It's an optimisation (trading a small bit of Python code for some C code) which seems to have very little gain. It's not like it's a user-facing API improvement/optimisation because this stuff is very low level.
Then wait until that is improved before making changes like this, which may end up being temporary changes that are no used once the "bigger" improvement is made. |
Unfortunately, we're past low-hanging optimizations which give big gain. Besides non-low-hanging big optimizations, there's also class of optimizations which you pinpoint well - those which low-hanging and provide "relative little" gain. But they still have their use: a) to clarify usecase/cover gaps; b) to uphold previously done optimizations. This is exactly one of them - the "service contract" for a scheduler is that it should not adversely affect the tasks running with it. A lot has went into making uasyncio scheduling not allocate memory by itself, and this is the change required to uphold that. So, even on "error paths" their should not be exception throwing, and this change is along that way. (Well, it just makes the existing behavior of such to blend with CPython's behavior with is not.) |
This is also pretty much how I feel (wary) about some of the further design and development to undertake. Bit again, unfortunately we're almost out of path which lead straight to the needed goal. You know that my approach was always on the side of "don't add stuff which may need to be removed" (ports, etc.) - but of course, that doesn't mean I wouldn't get into that situation myself even with all the caution I put into it. It's better to cover the gaps and remove technical debt, even if something might need to be deprecated later. I'm still extra cautious about that stuff, I just have to accept that sometimes the best solution is not the single path, but adding choices/dichotomies. For example, I'm now almost sure that there should be parallel extended functionality added to both of memory vs stream buffers, so users have a choice for each particular situation, even if the selection ends up skewed in 90%/10% ratio. So, here, I'm as the original designer/implementer of these things, sure that's the right addition. And then there's a practical side - I'm happy to wait here, but I need to continue work and evolve on my side. Which means updating uasyncio. So, this is my move to avoid unneeded discrepancies for the community (community complained). And you make yours. |
Some additional insight in the change made here: it has the same purpose as:
Here, poll.unregister() is also desired to not raise exception, but as the method itself doesn't return anything, the added param is boolean |
First of all, thank all of you for your volunteer work on open source. It's amazing that I can even run python on microcontrollers, and it's opened up some new avenues for the commercial makerspace I'm working with. I'm told that pfalcon/picoweb#45 is your fault. I'd appreciate it if you guys fought this out, because right now when I google "micropython web framework" and try to run the first example, it outright crashes. That's embarrassing, and makes micropython look like an immature platform. I'd like to be able to offer kits based around micropython, but this is the kind of thing that causes people to tell me we should just stick with arduino. I don't know where or how this should be fixed, but I do think you should spend some more time getting this sorted out. |
Closing. As mentioned the goal is not to diverge too much with CPython, it makes writing code that runs on both uPy and CPy more difficult. |
So this means uasyncio isn't going to work under standard micropython? Can we get an alternative? |
The version of uasyncio available at https://github.com/micropython/micropython-lib, and installable via upip, works with standard MicroPython. The plan is to maintain and enhance this uasyncio. |
Good to know that micropython has the standard namespace. |
@dpgeorge I would like that @pfalcon back to enhance the official uasyncio instead has a fork for it, where that fork is not compatible with official MicroPython. Please, let me explain: for the user, this is so difficult to understand, and worse, we need to create a PyCopy package just to use the a uasyncio with new features. We know that the uasyncio (like as asyncio on Python) is a very nice way to write concurrent tasks, and I believe that the MicroPython Project has so much to grown, but we need to have to continue to enhance the official uasyncio. The point is: are there plans to backport the features implemented by @pfalcon on him uasyncio lib to the official uasyncio lib? Sorry, but I need to talk what I'm feeling: that's (a fork for uasyncio) to me is not necessary, because @pfalcon are always maintain updated the pycopy from the last official MicroPython changes and official uasyncio can (maybe in the feature) maintain updated from @pfalcon uasyncio. That's for the user is not make sense. Why that if we can work together? Please, think about! Please, think for the community. Actually we have a official uasyncio lib where is not have new features over two years, and we have a person (the @pfalcon) that are hungry to evolution the uasyncio. Like a marriage, never we always agree with each other, so is not necessary to be 100% for decision A or B. I understand that is better to have a just a code for MicroPython and CPython, but if is not possible for now, not problem, we need to use people that want to contribute. I'm not talking about to forget the compatibility, but if is not possible to do now, in the feature we can to do. I not understand about the complexity for that, and maybe I'm talking just bullshit. I'm a python and micropython lover and I'm feeling a bit sad when I see that we (the community) is not capable to have a consensus. I agree that MicroPython has exceptional people working, and I just would like to pass the message that everyone together will to do the best for this project. Thank you. |
hi @beyonlo Completely agree, asyncio is a truly excellent feature of Python (and MicroPython). The situation we've found ourselves in is somewhat unfortunate, but I think a lot of the difficulty actually comes from the way that Anyway, driving a consensus is important, and to that end, somewhat driven by the discussion around #5275 & #5242 there are some massive improvements (by @dpgeorge ) to asyncio-support on MicroPython in the works. The result is a reworked-from-scratch implementation of uasyncio.py. I hope to see this become part of the default firmware images in a future release -- as I've mentioned in other threads, I'm personally keen to see this maybe be useful for more low-level functionality like DMA. i.e. it will be a proper officially supported part of core MicroPython, not just an optional and confusing thing that has to be installed separately. So, there are no plans to continue to maintain or backport-from-pycopy or do anything for the existing micropython-lib/uasyncio. Instead, the plan is to work on making this new version supported and useful for more people, and this is where it's at so far:
Anyway, I'm sure @dpgeorge will have more to say about this soon, he's been working hard on it! But like all things MicroPython and open source, there are lots of competing priorities. |
You mean massive mess? If anyone really intends to make "massive" changes to From the conversations in #5275 & #5242 I got the feeling people do not understand [1] It has been tried before and @pfalcon put a lot of effort into it. @dpgeorge, is the plan for "massive improvements" real and outlined somewhere? Can we get an official roadmap? Are these changes going to be in a separate |
In this case the existing |
@dpgeorge, I do think the However, unlike pushing a camel through the eye of a needle, the module name change is actually easy to do and I do not think it is a bad idea moving forward. So I am wondering if, despite not having any clout around here, I could ask you if my approval - which of course you do not need 😉 - for the name change could be exchanged for re-instating Note that while I'd like to keep |
Please, the naming is the smallest of problems.
We just need to put in documentation/warning that is not code compatible, and each user will research what works on the uasyncio and asyncio. I know that you core developers think about concepts, and that maybe make sense has a different name because is not the same concept than asyncio, but we need to think about most people that will use the MicroPython project: they are not core developers, are just simple users that want to do simple things, and after see that simple things works, they will start to research more and more. Now after discuss about angels sex, we need to focus about what really matter, the future of uasyncio (copying some questions from @dxxb )
I think that uasyncio is the bridge between a tests/prototype using microPython and the use microPython in production mode for solid projects on microcontrollers. So, I think we need to have a special dedication to uasyncio, because uasyncio is one pillar of the microPython project, like as asyncio is a pillar on CPython. Becouse everything that, I think that @dpgeorge could talk with @pfalcon to help us with official uasyncio, in a new roadmap, new features and so on. Like as I already told you, uasyncio has no new features a long time. And more, like I told above, when new modules are being creating, that should be created thinking in uasyncio, and more people working on uasyncio can help this new module to be a uasyncio compatible. Thank you. |
…n-main Translations update from Weblate
If 2nd, positional only argument of False is passed to poll.unregister()
method, then it won't throw exception if passed a stream object not
registered with the poller, which was the previous behavior. Then default
behavior is made compatible with CPython's, which is to throw exception
in this case.