Skip to content

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

Closed

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Oct 6, 2018

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.

Paul Sokolovsky added 3 commits October 6, 2018 21:29
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.
@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 6, 2018

This is a follow-up to #4197, implementing handling as proposed in #1550 (comment)

@dpgeorge
Copy link
Member

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.

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 17, 2018

But it's not clear to me that unregister() needs to be extended to have the proposed "throw" argument.

It adds idempotent behavior, which is always a useful feature.

Divergence from CPython should be minimised where possible, and in this case it adds code to diverge.

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:
micropython/micropython-lib#77 (comment) .

Here, poll.register() provides useful idempotent behavior, and poll.unregister() accidental-gaply don't. So, we fix it in compatible, natural way.

And uasyncio doesn't seem to need this behaviour since it already tracks what is in the poller with objmap.

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.

@dpgeorge
Copy link
Member

Here, poll.register() provides useful idempotent behavior, and poll.unregister() accidental-gaply don't. So, we fix it in compatible, natural way.

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?

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 17, 2018

CPython seems fine without such a fix.

It's a weird argument to bring in. Turned out, that CPython is not fine without :=, while it (and everyone) was fine without it for 25 years. So what?

As I mentioned, objmap is a "dirty" workaround and supposed to go. Exceptions are expensive and allocate memory.

for which there is only one use case so for?

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.

@dpgeorge
Copy link
Member

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.

As I mentioned, objmap is a "dirty" workaround and supposed to go.

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.

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 18, 2018

It's an optimisation (trading a small bit of Python code for some C code) which seems to have very little gain.

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.)

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 18, 2018

making changes like this, which may end up being temporary changes that are no used once the "bigger" improvement is made.

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.

@pfalcon
Copy link
Contributor Author

pfalcon commented Dec 27, 2018

Some additional insight in the change made here: it has the same purpose as:

  • .get() method of dictionaries comparing to just subscribing [] - avoiding KeyError exception and returning None (or other default).
  • default param to dict.pop(key, [default]) - again, avoiding exception, returning a value instead.

Here, poll.unregister() is also desired to not raise exception, but as the method itself doesn't return anything, the added param is boolean throw, instead of default.

@traverseda
Copy link

traverseda commented Feb 23, 2019

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.

@dpgeorge
Copy link
Member

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.

@dpgeorge dpgeorge closed this Oct 16, 2019
@traverseda
Copy link

So this means uasyncio isn't going to work under standard micropython? Can we get an alternative?

@dpgeorge
Copy link
Member

So this means uasyncio isn't going to work under standard micropython?

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.

@traverseda
Copy link

Good to know that micropython has the standard namespace.

@beyonlo
Copy link

beyonlo commented Nov 5, 2019

So this means uasyncio isn't going to work under standard micropython?

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.

@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.

@jimmo
Copy link
Member

jimmo commented Nov 6, 2019

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 uasyncio isn't really quite the same as asyncio. I don't want to speak much to the pycopy / pfalcon situation, other than to say that I think that it is just a distraction from the actual goal -- to make asyncio work well on MicroPython in a way that is familiar and useful for Python programmers.

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:

  • Runs much more CPython asyncio code directly (i.e. asyncio.run, asyncio.gather, proper Tasks, Events, cancellation, etc) while still maintaining efficiency and performance.
  • Runs existing uasyncio code without modification (e.g. microdot, picoweb, etc)
  • Requires fewer micropython-specific extensions (e.g. pend_throw, utimeq, etc) -- so these could be removed in a future release reducing code size for people who don't want asyncio.
  • Handles recent improvements like modlwip.c does not properly return POLL_HUP and POLL_ERR socket errors #5172

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.

@dxxb
Copy link
Contributor

dxxb commented Nov 6, 2019

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.

You mean massive mess? If anyone really intends to make "massive" changes to uasyncio to make it more compatible to CPython's asyncio, please create a new asyncio module and leave uasyncio users alone. The uasyncio module, despite the similarity in naming with asyncio, has a different design that cannot fit CPython's asyncio way of doing things. Users of uasyncio such as myself, and the company I work for, have code in production that relies on uasyncio not being broken.

From the conversations in #5275 & #5242 I got the feeling people do not understand uasyncio well but are not afraid to make changes to it and are underestimating the issues involved with making uasyncio more like asyncio [1] while still running on constrained devices and behaving correctly (and any implementation that doesn't allow a coroutine to handle injected exceptions under some circumstances is not well-behaved).

[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 asyncio module (i.e. a different namespace)? Should I expect more "pushy, API-breaking happy" attitude like in #5275 & #5242?

@dpgeorge
Copy link
Member

dpgeorge commented Nov 6, 2019

If anyone really intends to make "massive" changes to uasyncio to make it more compatible to CPython's asyncio, please create a new asyncio module and leave uasyncio users alone. The uasyncio module, despite the similarity in naming with asyncio, has a different design that cannot fit CPython's asyncio way of doing things.

In this case the existing uasyncio module should not be called uasyncio because that name is reserved for an asyncio-compatible module (like how other modules with a u-prefix work). Eg it would make sense to rename it to something like uasched.

@dxxb
Copy link
Contributor

dxxb commented Nov 6, 2019

@dpgeorge, I do think the uasyncio naming is unfortunate being incompatible with CPython asyncio and yet has a name that suggest being a limited version of the same. On the other hand, and for the sake of honesty, I also feel that the late claim of namespace reservation is a bit like trying to push the camel through the eye of the needle (forced and comical because it feels like a thinly diguised excuse to me), but I realise it might just be me.

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 .pend_throw() to pre-#5275 behaviour and your assurance that within reason the rug won't be pulled from under uasched for the forseeable future by removing or changing required undelying mechanism (like .pend_throw()).

Note that while I'd like to keep uasched working, what I care about is having a way to implement small and well behaved alternative schedulers using generators if the need arises.

@beyonlo
Copy link

beyonlo commented Nov 6, 2019

Please, the naming is the smallest of problems.
In my opinion the name uasyncio is very fine, and more, that name is really the best way to naming this module. Let me explain:

  1. People that come from CPython with asyncio, is easy to understand that MicroPython has a asynchronous module (uasyncio), like as the asyncio on CPython.
  2. People that start first with MicroPython than CPython, when start with CPython will know that there are a asynchronous module (asyncio), like as the uasyncio on MicroPython.

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 )

  1. "Is the plan for "massive improvements" real and outlined somewhere?"
  2. "Can we get an official roadmap?"
  3. Are there plans to always when a new module/feature will be is implement, implement it thinking in truly concurrent mode/non-blocking to works with uasyncio?

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.

kamtom480 pushed a commit to kamtom480/micropython that referenced this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants