Refactor module (un|re)loading #1053

calebj · 2016-03-30T10:38:14Z

Commit summary stolen for use as PR description, since @calebj didn't enter one. — @dgw

Core changes:
Now there's a single place to (un)register modules and keep track of
registered modules, called by both reload.py and bot.setup(). Please
let me know if I forgot to unload/reset something. It hasn't broken
on me yet.

The command categories are structured slightly differently. Instead
of storing only the first command alias, it stores a list of all of
them. I also changed the help.py module to work with this structure.

I added an unload command to the reload module. Since the bot uses a
seperate structure to store loaded modules, the user can't do something
stupid like 'unload sopel'.

Scheduler changes:
Not much here, just added another method that removes a job. Sadly,
PriorityQueue doesn't support this natively, so I just rebuild the
queue, skipping the job in question. This is used in bot.unregister()

Reload module changes:
Since the command is included in trigger.groups(2), making seperate
rules for PM and nick commands that call the same function won't work
without ignoring the first word when triggered by nickname_commands.
It's easier to make it use standalone command syntax, which works in
both PMs and in a channel.
Also, I replaced the admin tests with their decorator counterparts.

calebj · 2017-11-21T15:46:01Z

Dunno if this patch still works, but I rebased it anyway. Will probably test later today.

dgw · 2018-03-27T06:31:47Z

@calebj Are you still available to work on this/#1054? They'll need some rebasing work to catch up with changes to the help module made in the mean time, so I'm holding off on code review for now.

calebj · 2018-03-27T13:59:06Z

Yeah, I'm still available. Should be rebased now.

dgw · 2018-03-29T18:18:08Z

Sorry @calebj, needs rebasing again. 😅

Could you test whether/how this affects #1056? If this helps at all, even if it doesn't fix all the issues, it might be a big step just the same. Also curious how this interacts with #990, if at all.

No rush on this, it's all future stuff not scheduled to be in any release yet.

dgw

I've left some line notes for specific things, but my overall impression from running this is not great at the moment. It made the duplicated-command behavior from #1056 worse: Commands from reloaded modules now output three times instead of twice.

04:20:27 <~dgw>  ,wa dgw
04:20:34 <Sopel>  [W|A] Converse County Airport = Douglas, Wyoming, United States
04:20:34 <~dgw>  ,reload wolfram
04:20:36 <Sopel>  dgw: <module 'wolfram' from '/usr/lib/python3.6/site-packages/sopel_modules/wolfram/__init__.py'> (version: 2018-04-06 09:11:42)
04:20:42 <~dgw>  ,wa dgw
04:20:45 <~dgw>  how many...
04:20:49 <Sopel>  [W|A] Converse County Airport = Douglas, Wyoming, United States
04:20:50 <Sopel>  [W|A] Converse County Airport = Douglas, Wyoming, United States
04:20:51 <Sopel>  [W|A] Converse County Airport = Douglas, Wyoming, United States

At a bare minimum, for this to get merged it must not cause any regressions. Fixing something would be good too, though. 😁 And as mentioned in a line note, the "command groups" bit should probably become its own PR, so this one doesn't turn into too much of a hydra. I understand if the help changes require this refactor before they can work, but fixing the reloading issues is much more important at the moment.

dgw · 2018-04-06T09:22:08Z

sopel/bot.py

+    def unregister_module(self, module):
+        for obj_name, obj in iteritems(vars(module)):
+            self.unregister(obj)
+        callables, _, _ = sopel.loader.clean_module(module, self.config)


This causes a ValueError; clean_module() only returns one value.

dgw · 2018-04-06T09:23:24Z

sopel/modules/help.py

@@ -63,6 +63,7 @@ def help(bot, trigger):
            for category, cmds in collections.OrderedDict(sorted(bot.command_groups.items())).items():
                category = category.upper().ljust(name_length)
                cmds = set(cmds)  # remove duplicates
+                cmds = ['|'.join(cg) for cg in cmds]


Maybe let's split command-group help into its own PR?

dgw · 2018-04-06T09:24:31Z

sopel/modules/reload.py



-@sopel.module.nickname_commands("reload")


I'm not a fan of changing the command behavior just for the hell of it. Sopel: reload modulename should continue to work.

dgw · 2018-04-06T09:29:49Z

sopel/modules/reload.py

@@ -33,54 +32,40 @@ def f_reload(bot, trigger):
            'medium': collections.defaultdict(list),
            'low': collections.defaultdict(list)
        }
+        bot.shutdown_methods.clear()
+        bot.scheduler.clear_jobs()


With the refactor in bot.py to remove only the specified job, why is the reload module now clearing the whole queue? Why is reload clearing all shutdown methods for the entire bot instead of only the specified module? Keeping #831 in mind, this refactor is a good opportunity to stop clearing things that shouldn't be.

calebj · 2018-06-11T16:30:56Z

I think reloading should work for you now. Both of my PRs are having issues with that URL check, but it tests fine locally. I would like to squash and add a 'fixes' keyword if these are acceptable to merge.

dgw · 2018-06-11T16:55:05Z

Kicked Travis, test passed. Bing can be an utter diva. It'd be easier to fix if it failed consistently…

Squashing sounds great! But not just yet, sorry. Don't have as much time as I'd like right now to go back and do another in-depth test of this, and the moved functions make even reviewing the diff hard. I will, however, put this (and your other rebased PR) at the top of my to-do list, so it shouldn't be more than a few days before I have a chance to look at both of them. 😸

dgw · 2018-06-13T16:50:38Z

Currently blocked on #1343, a regression caused by me fulfilling the first half of my promise and merging #1052 without (apparently) testing it as thoroughly as I should have. Sorry @calebj, I'll still get to this ASAP.

Requested changes addressed. Larger direction being discussed before next review.

calebj · 2018-08-18T23:43:58Z

Rebased, and appears to be working with core modules, wolfram and youtube on Python 2.7, 3.6, and 3.7 🎉

I implemented some of the ideas from #1314, but used code I adapted from an existing module in another project. This should fix #1324 and, of course, #1056. #990 still applies cleanly.

@ralienpp, would you mind testing this branch with your own modules?

ralienpp · 2018-08-19T10:17:20Z

No luck yet, @calebj. Here's what I did, just to make sure we're on the same page:

Install that particular commit you're talking about: pip install git+https://github.com/sopel-irc/sopel.git#a970dee4336133bc5c5c9dece33c5b41301c8bed --upgrade
restart sopel
run a command from my module, confirm that I only get one response, as expected
send .reload <my module name>
run command from my module again - observe that I get 2 responses = not good.

I sent a pull request that adds the module in question to the built-in set; the functions are quite simple in their nature: #1366 Most of the logic in my headless systems (updates, status checks, etc) is implemented through the execution of programs via this .run command.

calebj · 2018-08-19T16:34:58Z

Weird. That happens on my end too... but not if I run it from the repo, or pip install from the repo folder. Something isn't making it to the repo, even though it says my working tree is clean. Will investigate.

calebj · 2018-08-19T16:40:44Z

Found it. You used # when you should have used @. Running this:

pip install git+https://github.com/sopel-irc/sopel.git@a970dee4336133bc5c5c9dece33c5b41301c8bed --upgrade

works as expected. You should get a Reloaded: ... message after the module spec and mtime.

ralienpp · 2018-08-19T20:47:07Z

This is what I get when I run the command with the # symbol replaced by a @:

/pip install git+https://github.com/sopel-irc/sopel.git@a970dee4336133bc5c5c9dece33c5b41301c8bed --upgrade
Collecting git+https://github.com/sopel-irc/sopel.git@a970dee4336133bc5c5c9dece33c5b41301c8bed
  Cloning https://github.com/sopel-irc/sopel.git (to a970dee4336133bc5c5c9dece33c5b41301c8bed) to /tmp/pip-YpPR5y-build
  Could not find a tag or branch 'a970dee4336133bc5c5c9dece33c5b41301c8bed', assuming commit.
fatal: reference is not a tree: a970dee4336133bc5c5c9dece33c5b41301c8bed
Command "git checkout -q a970dee4336133bc5c5c9dece33c5b41301c8bed" failed with error code 128 in /tmp/pip-YpPR5y-build

This has happened before, please see the history here: #1056 (comment)

calebj · 2018-08-19T20:53:59Z

Try passing it my fork URL instead:

pip install --user git+https://github.com/calebj/sopel.git@a970dee4336133bc5c5c9dece33c5b41301c8bed --upgrade

ralienpp · 2018-08-19T22:05:22Z

I had to adjust your command and remove the --user part, to install it. The experiment was successful, I reloaded it a couple of times - still got the expected behaviour. Thanks!

I will let this picture express my feelings in a more dramatic way https://cdn.theatlantic.com/static/mt/assets/science/JPL.gif

dgw · 2018-08-20T20:43:29Z

Hey @calebj, any ideas on this behavior?

15:27:57 <~dgw>  Sopel: reload wolfram
15:27:58 <Sopel>  dgw: <module 'wolfram' from '/home/dgw/.sopel/modules/wolfram/__init__.py'> (version: 2018-08-20 20:27:55)
15:27:59 <Sopel>  Reloaded: wolfram
15:28:01 <~dgw>  .wa2
15:28:01 <Sopel>  wa2 command added
15:28:11 <~dgw>  Sopel: reload wolfram
15:28:12 <Sopel>  dgw: <module 'wolfram' from '/home/dgw/.sopel/modules/wolfram/__init__.py'> (version: 2018-08-20 20:28:09)
15:28:12 <Sopel>  Reloaded: wolfram
15:28:18 <~dgw>  (removed wa2 command)
15:28:20 <~dgw>  .wa2
15:28:20 <Sopel>  wa2 command added
15:28:21 <Sopel>  wa2 command added
15:28:22 <Sopel>  wa2 command added
15:28:23 <Sopel>  wa2 command added
15:28:23 <Sopel>  ...

(The last ... is the repetition protection blocking out a fifth copy of the same line.)

In this test, .wa2 triggered the following:

@commands('wa2')
def wa2(bot, trigger):
    bot.say('wa2 command added')

And to test removal, I simply removed the whole function and its decorators.

FWIW, my reloading overhaul in #1314 also bugs out on removed methods, but it only adds one repetition. Yours seems to add four, and I'm not sure why. (Not that I'm yet sure why mine adds even one…)

calebj · 2018-08-20T23:39:00Z

The whole practice of modifying the command function object directly isn't the best thing to work with, but I fixed it in my latest push.

In a future update, I would advocate moving to a single @command decorator that takes keyword arguments for rule= and so on. The decorator would then create a new object with the necessary attributes and leave the original function object alone.

dgw · 2018-08-21T00:30:40Z

I'm more than a little squeamish about doing anything like del sys.modules[name]… https://justus.science/blog/2015/04/19/sys.modules-is-dangerous.html comes to mind.

Reloading support is probably not worth completely rejiggering the decorators, honestly. Keeping multiple versions of callable objects around doesn't tickle my fancy either. :/

calebj · 2018-08-21T00:34:19Z

The handling here has been robust in my experience, and only Sopel modules are removed from sys.modules. Full reloading needs a full GC due to the modifications made to the function objects directly, and the cached item in sys.modules prevents that.

dgw · 2018-08-21T01:34:12Z

I need to dig way into this at a computer, hmm. There must be a good way to have our cake and eat it too re: modifying functions during load…

ralienpp · 2018-10-04T08:49:50Z

Having used that patched version for a while, I have found that the problem persists, unfortunately. My first thought is that the initial test, where I use .reload to trigger the reloading action - is not enough. That test passes, but over time something else happens that leads to this behaviour.

What other testing technique could I try?
Is there something else I can do to help speed up the resolution of this issue?

dgw · 2018-10-04T21:54:21Z

@ralienpp Does the problem persist with #1314 as well? Because if not, I'm happy to merge that for inclusion in the next release and let work on this refactor continue for the future.

dgw · 2019-02-15T23:43:34Z

So, @HumorBaby is putting a lot of work into the reloading logic, and @Exirel is working on refactoring the plugin/module system itself. Between the two of them, they're likely to duplicate a lot of this PR.

Given that this PR has kind of stalled, I'm going to say it'll wind up getting closed unmerged unless @calebj reappears to continue work on it relatively soon.

dgw · 2019-04-04T15:54:41Z

Closing in favor of #1479

calebj force-pushed the refactorreload branch 4 times, most recently from fc1e194 to a604e3f Compare April 2, 2016 23:51

maxpowa added the Needs Review label Sep 30, 2016

maxpowa mentioned this pull request Sep 30, 2016

Add help command that displays docs for all commands in a category #1054

Closed

calebj force-pushed the refactorreload branch from a604e3f to ba63dda Compare November 21, 2017 15:26

calebj force-pushed the refactorreload branch from ba63dda to c2d016a Compare March 27, 2018 13:45

dgw self-requested a review March 27, 2018 14:48

dgw mentioned this pull request Mar 29, 2018

Jobs weird on reload? #831

Closed

dgw previously requested changes Apr 6, 2018

View reviewed changes

dgw added Medium Priority and removed Needs Review labels Apr 6, 2018

dgw added this to the 6.6.0 milestone May 16, 2018

calebj force-pushed the refactorreload branch 3 times, most recently from 0ac67a5 to 72dd79e Compare June 11, 2018 16:19

dgw added the Needs Review label Jun 11, 2018

dgw self-assigned this Jun 11, 2018

This was referenced Jun 12, 2018

reload: Recurse into packaged modules #1314

Merged

Nickname commands have wrong capture groups if alias_nicks set #1343

Closed

dgw mentioned this pull request Jun 22, 2018

reload: Reloading packaged modules doesn't work correctly #1056

Open

2 tasks

calebj changed the title ~~Refactor module (un|re)loading and add help for entire command categories~~ Refactor module (un|re)loading Aug 18, 2018

make codacy happy

dc2c4c9

dgw self-requested a review August 19, 2018 23:35

fix module removal

6d66ab0

dgw removed their request for review February 15, 2019 23:40

dgw added Stale Mostly used for PRs that no longer work and need updating before re-review/merge. and removed Needs Review labels Feb 15, 2019

dgw removed this from the 7.0.0 milestone Feb 28, 2019

HumorBaby mentioned this pull request Feb 28, 2019

reload: fix to handle removed callables + cleanly remove intervals + missing priority error #1399

Closed

dgw mentioned this pull request Apr 3, 2019

plugin: new interface #1479

Merged

dgw closed this Apr 4, 2019

dgw added Replaced Superseded by a newer PR and removed Stale Mostly used for PRs that no longer work and need updating before re-review/merge. labels Apr 4, 2019

dgw removed their assignment May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor module (un|re)loading #1053

Refactor module (un|re)loading #1053

calebj commented Mar 30, 2016 •

edited by dgw

Loading

calebj commented Nov 21, 2017

dgw commented Mar 27, 2018

calebj commented Mar 27, 2018

dgw commented Mar 29, 2018

dgw left a comment

dgw Apr 6, 2018

dgw Apr 6, 2018

dgw Apr 6, 2018

dgw Apr 6, 2018

calebj commented Jun 11, 2018

dgw commented Jun 11, 2018

dgw commented Jun 13, 2018

calebj commented Aug 18, 2018

ralienpp commented Aug 19, 2018 •

edited

Loading

calebj commented Aug 19, 2018

calebj commented Aug 19, 2018 •

edited

Loading

ralienpp commented Aug 19, 2018

calebj commented Aug 19, 2018

ralienpp commented Aug 19, 2018

dgw commented Aug 20, 2018

calebj commented Aug 20, 2018

dgw commented Aug 21, 2018

calebj commented Aug 21, 2018 •

edited

Loading

dgw commented Aug 21, 2018

ralienpp commented Oct 4, 2018

dgw commented Oct 4, 2018

dgw commented Feb 15, 2019

dgw commented Apr 4, 2019



		@sopel.module.nickname_commands("reload")

Refactor module (un|re)loading #1053

Refactor module (un|re)loading #1053

Conversation

calebj commented Mar 30, 2016 • edited by dgw Loading

calebj commented Nov 21, 2017

dgw commented Mar 27, 2018

calebj commented Mar 27, 2018

dgw commented Mar 29, 2018

dgw left a comment

Choose a reason for hiding this comment

dgw Apr 6, 2018

Choose a reason for hiding this comment

dgw Apr 6, 2018

Choose a reason for hiding this comment

dgw Apr 6, 2018

Choose a reason for hiding this comment

dgw Apr 6, 2018

Choose a reason for hiding this comment

calebj commented Jun 11, 2018

dgw commented Jun 11, 2018

dgw commented Jun 13, 2018

calebj commented Aug 18, 2018

ralienpp commented Aug 19, 2018 • edited Loading

calebj commented Aug 19, 2018

calebj commented Aug 19, 2018 • edited Loading

ralienpp commented Aug 19, 2018

calebj commented Aug 19, 2018

ralienpp commented Aug 19, 2018

dgw commented Aug 20, 2018

calebj commented Aug 20, 2018

dgw commented Aug 21, 2018

calebj commented Aug 21, 2018 • edited Loading

dgw commented Aug 21, 2018

ralienpp commented Oct 4, 2018

dgw commented Oct 4, 2018

dgw commented Feb 15, 2019

dgw commented Apr 4, 2019

calebj commented Mar 30, 2016 •

edited by dgw

Loading

ralienpp commented Aug 19, 2018 •

edited

Loading

calebj commented Aug 19, 2018 •

edited

Loading

calebj commented Aug 21, 2018 •

edited

Loading