Skip to content

Feature Request: pyscript.reload to support new files. #106

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
dlashua opened this issue Dec 2, 2020 · 9 comments
Closed

Feature Request: pyscript.reload to support new files. #106

dlashua opened this issue Dec 2, 2020 · 9 comments

Comments

@dlashua
Copy link
Contributor

dlashua commented Dec 2, 2020

Currently, calling pyscript.reload with a global_ctx= set to a newly created file results in an error indicating the global context cannot be found, which makes sense because a full reload hasn't happened yet so that global context has never been loaded, and, therefore, can't be reloaded.

For deleted files, pyscript.reload works just fine. The global_ctx is unloaded and, since the file no longer exists, is not reloaded.

It would be great if reload could look for the new file and cause it to load. Optionally, pyscript.load could be used instead to establish a difference between a new file and a reload. Though, if this happens, I suggest that reload does not work on deleted files and a pyscript.unload gets added.

This is a nice stepping stone to having full autoREload support in pyscript since this functionality will be needed for that.

@craigbarratt
Copy link
Member

craigbarratt commented Dec 2, 2020

Good points. Actually, how about reload (with no params) looks for differences (comparing file contents and maybe mtimes, and checking for new and deleted files) and just makes the minimum changes - unloads deleted files, reloads changed files, and loads new files? Files with no changes aren't touched.

The existing global_ctx param could be a force reload (even if no changes), and also handle the new file case as you mention. We could also use global_ctx='*' to force reload everything.

It makes the default of plain reload do what you want 99% of the time (although it would be a breaking change).

Once #74 is implemented on linux, then reload would become largely irrelevant. But the improvements above would be useful for non-linux hosts.

@dlashua
Copy link
Contributor Author

dlashua commented Dec 3, 2020

That is EVEN BETTER! This way, if something changes, just call reload and it will do what is needed.

And yes, on Linux, with #74, reload won't be needed manually. But, the logic inside reload is likely the SAME logic that will be needed when #74 detects a change. So the code itself will still be used.

Some harder questions: YAML and modules.

When a module (in pyscript/modules) changes, a reload to modules.filename reloads the module, but any existing scripts using that module hold on to the old copy until they are also reloaded. If modules.filename is NOT reloaded, then reloading the script file does NOT cause the new module file to be used. To sum up, both the module AND the script using the module need to be reloaded in order to really see the module changes. Since pyscript already dips into how "import" works, is there a way to track the imports each script uses and, if that import is a module and that module is reloaded, also reload the script?

When YAML changes, reloading JUST the script that uses the YAML results in that script seeing the new YAML. This is great. Is there anyway to detect when YAML changes? If so, could the old YAML be compared to the new YAML and if changes are detected at pyscript/apps/appname then apps.appname would be reloaded as well? This doesn't affect me directly in most cases, because I use a different YAML format than the convention suggested by pyscript because I like to separate my YAML by functionality. So I use pyscript/apps_list/[{app: appname, a: 1}, {app: appname, a: 2}] format instead. I think the only way to get MY YAML usage to autoreload would be some kind of YAML registration function called in the app code to indicate which parts of YAML it uses. But I can write that in user code if there's a way to detect YAML changes.

For YAML changes, what if there was a @time_trigger('reload')? Somewhere during pyscript.reload new YAML is picked up. Even if pyscript is unable to detect changes, if @time_trigger('reload') was triggered on ANY reload, then user code could pull in YAML, compare as needed and call pyscript.reload on anything that needed reloading. This way, if I change the YAML, I only need to call pyscript.reload manually to get those changes to be picked up and all the things that need to reload could do so.

@craigbarratt
Copy link
Member

craigbarratt commented Dec 7, 2020

I just pushed a significant rewrite of reload. It now only reloads changes, and changes include an app's configuration as well as new or deleted files. A changed module/package causes any scripts, apps or other modules to be reloaded too if they import (directly or indirectly) the changed module.

While this is a breaking change, the old behavior can be forced with global_ctx set to *.

@dlashua
Copy link
Contributor Author

dlashua commented Dec 7, 2020

!!!! Thank you!

I'll pull "master" this morning and test it.

@dlashua
Copy link
Contributor Author

dlashua commented Dec 7, 2020

This worked perfectly with everything I threw at it.

  • changes made in pyscript/scripts/somefile.py
  • changes made in pyscript/somefile.py
  • changes made in pyscript/apps/someapp/init.py
  • changes made in pyscript/modules/somemodule.py (which also reloaded scripts that imported it)
  • commenting out (#) a file name
  • commenting out (#) a directory name
  • creating a new file
  • deleting a file

In each case, above, after making changes, I only called "pyscript.reload" with no parameters.

The only request I have, and it's not a big deal, is... when a script is loaded a message of "Loaded /config/pyscript/test.py" is generated. This is useful to know that something loaded (or reloaded). However, when a script is deleted (so, unloaded and then NOT reloaded), there is no confirmation in the log.

Adding a "Unloaded /config/pyscript/test.py" log message would be useful. If this happens before the "Loaded" log line on a reload (because it is both unloaded and then loaded) that's okay. Optionally, the log could say "Loaded", "Unloaded", or "Reloaded", depending on which of the three things are happening.

@dlashua
Copy link
Contributor Author

dlashua commented Dec 7, 2020

Another thing I just tested that worked perfectly...

If a script has an exception during load (not in a trigger function, but in the code that defines trigger functions), then pyscript does NOT count it as loaded. So the next call to reload will try to load that file again, even if no changes have been made to the file.

I tested it like this:

CNT = 0

if pyscript.manually_set == "on":
    @time_trigger("period('2020-01-01',1)")
    def counter():
        global CNT
        CNT = CNT + 1
        log.info(f"7 Counter at {CNT}")

pyscript.manually_set did not exist in my system. Every time I issued pyscript.reload I'd correctly see:

Exception in </config/pyscript/test.py> line 3:
    if pyscript.manually_set == "on":
       ^
NameError: name 'pyscript.manually_set' is not defined

Once I manually set that entity and value, I issued pyscript.reload again and the script started up as desired (though I didn't actually expect it to work, so that was a bonus).

@dlashua
Copy link
Contributor Author

dlashua commented Dec 7, 2020

I was able to reduce the complexity of my own file change "watchdog" script thanks to these changes to reload. Together, they are working exactly as expected.

https://gist.github.com/dlashua/f7d88f9a5afdcf7af17ce24266925a0b

@dlashua
Copy link
Contributor Author

dlashua commented Dec 7, 2020

Reworked the watchdog to use aionotify instead. This should be easier to incorporate into pyscript core if desired.

https://gist.github.com/dlashua/f7d88f9a5afdcf7af17ce24266925a0b

The except OSErrors are there because I couldn't get it to be reliable if it was going to flag a renamed directory with MOVE_SELF, MOVED_FROM, or BOTH, and the second attempt to "unwatch" would fail because it was no longer being watched because of the first attempt.

@craigbarratt
Copy link
Member

I pushed a commit that now logs Loaded/Unloaded/Reloaded. Note that a change to something that is imported (eg, a module or a sub-file in an app) will separately report Unloaded then Loaded (rather than a single Reloaded), since reload just unloads those files and they only get loaded later when the code executes the import (eg, it could be the new code doesn't even do the import anymore, in which case you would see Unloaded and no Loaded).

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

No branches or pull requests

2 participants