Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Mar 23, 2021

Description

Given the numerous issues with trying to live-reload plugins that live in namespace packages or entry-points, I have started on making the reload plugin act only on plugins that it's likely to successfully reload.

The approach might be stupid. Or maybe it's brilliant. Possibly, nothing will make sense, since I wrote this in bits and pieces over several brief coding sessions spanning at least a week.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

Not exhaustively tested yet. I finished the last bit of stuff I wanted to write before opening a PR just before (actually just after) the checkout deadline for our hotel. 😁

@dgw dgw added the Feature label Mar 23, 2021
@dgw dgw added this to the 7.1.0 milestone Mar 23, 2021
@Exirel
Copy link
Contributor

Exirel commented Apr 29, 2021

I thought a lot about that. If I agree that we need to limit the reload, somehow, to only single file plugins, I'm not convinced by the plugin_types parameter.

If you push the idea further, there are 2 facts:

  • the reload plugin is, as far as we know it, the only one that uses reload methods
  • only single file plugins can be safely reloaded

As a result, I wonder if what we want is either to:

  • change bot.reloads to reload only plugin that can be reloaded—at the moment, single file only
  • change the reload plugin to manually reload every single file plugins

I know reloading is very tricky, and I'm really not happy with that feature. In particular, I'm always a bit annoyed that the only way to reload a plugin's configuration is to "reload" the plugin itself, which shouldn't be necessary.

@dgw
Copy link
Member Author

dgw commented Apr 29, 2021

The big question, then, is whether you still think it's possible to make reloading work reasonably well for other plugin types after we start on 8.x dev and drop all Python versions <= 3.5 (or <= 3.6, since it's likely to go EOL in Dec 2021 before 8.0 is ready for release). It's been quite some time since we talked about it; the last substantive comment I could find in my IRC logs is from two years ago:

./2019/02/16/freenode_#sopel.log.gz:183:
    [11:55:42] <Exirel> dgw: I've got a reason to switch to Py3.4 and above, it's the first version that expose
                        its public internals to reload module that allows to get rid of old definitions. :D

I could scrap this approach and implement your idea from a year ago instead 😁:

./2020/03/20/freenode_#sopel.log.gz:183:
    [19:47:37] <Exirel> All I need to add is a `can_reload` method on the abstract class, implement it to say
                        NO all the time but when it's a single-file.

Tangent: Sopel 8 can go bonkers with a lot of stuff because of dropping old Pythons. Both reloading and asynchat are up for refactoring/replacement in 3.6, according to you-from-18-months-ago:

./2019/11/20/freenode_#sopel.log.gz:208:
    [15:31:27] <dgw> 3.6 is where you get the tasty toys for making reload better, isn't it
    [15:31:51] <Exirel> Hm... something like that.
    [15:32:06] <Exirel> I think it's 3.4 for reload, and 3.6 for asyncio.

For my part, I'm already tired just thinking about reviewing the gIanT PuLl rEquEStS you'll create 😅 😛

@Exirel
Copy link
Contributor

Exirel commented Apr 29, 2021

reasonably well for other plugin types

I'm still not sure about that part. In theory, yes, there are the tools available to do way more than before, but who knows how it will actually behave in the real world?

However, I still think there is something to do that would still be useful: add a new hook (such as setup & shutdown), that would let the plugin itself to decide what to do on reload, with a default implementation just trying to reload the python file, in order to be backward compatible with today's version.


Oh yeah, I'm so ready for these. 😁

@dgw dgw modified the milestones: 7.1.0, 8.0.0 May 7, 2021
@dgw
Copy link
Member Author

dgw commented May 7, 2021

We will worry about this in 8.0 and beyond. Reloading has been semi-broken ever since plugin types beyond single-file were introduced, well before either of us got involved with Sopel. It can stay so for another release series, at least.

@dgw
Copy link
Member Author

dgw commented Feb 28, 2023

We've got other stuff about the plugin system to work on, like adding support for async. At this point I don't think it's worth spending any more time on reload logic, because future plans are likely to break it again.

@dgw dgw removed this from the 8.0.0 milestone Feb 28, 2023
@dgw dgw closed this Mar 2, 2023
@dgw dgw deleted the reload-only-single-file branch March 2, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants