Skip to content

Improve performance, streamline code in a LOT of places. Fixed bugs.#11

Merged
julvo merged 10 commits into
julvo:masterfrom
laundmo:master
Jun 1, 2021
Merged

Improve performance, streamline code in a LOT of places. Fixed bugs.#11
julvo merged 10 commits into
julvo:masterfrom
laundmo:master

Conversation

@laundmo
Copy link
Copy Markdown
Contributor

@laundmo laundmo commented Feb 27, 2021

Bugs fixed:

  • can only have a single reloading loop - now works with multiple
  • loop vars if reloading loop isn't first were wrong. - dumb regex looked at the first loop it could find, now getting vars using ast.

Performance improvements:

  • now allows for the keyword only argument reload_after, which will only reload the code every n iterations
  • reduced total number of inspect calls
  • compile() the code first, before running it

Reloading loop performance comparison:
image
r is the reload_after
old refers to whether this is the old version of the library.

The benchmark code is avaliable on laundmo:benchmark

This is heavily related to #10

Bugs fixed:
- can only have a single reloading loop - now works with multiple
- loop vars if reloading loop isn't first were wrong. - dumb regex looked at the first loop it could find, now getting vars using ast.

Performance improvements:
- now allows for the keyword only argument reload_after, which will only reload the code every n iterations
- reduced total number of inspect calls
- compile() the code first, before running it (only loops so far)
@laundmo laundmo marked this pull request as ready for review February 28, 2021 12:25
@julvo
Copy link
Copy Markdown
Owner

julvo commented Mar 1, 2021

Hey, thanks a lot for the PR 🙏 Will have a look through the code sometime this week

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Mar 1, 2021

hey, i need to test this some more, i remembered that i haven't tested how well errors are handled, more specifically, syntax errors.
i moved the compilation step outside of the loop/decorator, which may mean that syntax errors happen there and aren't caught by the existing error handler.

I will test this over the next few days, and comment the results.

sidenote: i use the black formatter, which is why the diff probably seems larger than expected. didn't mean to run it, but oh well.

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Mar 4, 2021

@julvo i have now reworked the way functions reload (no more loading specific line numbers!) and also fixed the issues with error handling, which was also streamlined some more. Its definitely ready for merge from my side, please do still test/review it.

at this point i've basically rewritten the entire thing 😆

EDIT: scratch that, i broke the normal looping. back to work.

EDIT: EDIT: fixed it

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Mar 7, 2021

I think I'm finally done. Unless I think of some amazing way to make it work with while loops, but there most likely isn't one.

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Mar 12, 2021

@julvo whats the status on this?

@julvo
Copy link
Copy Markdown
Owner

julvo commented Mar 15, 2021

I'm currently quite busy, but hope to get around to it it on the weekend. As it's quite a substantial change, I want to review and test this properly. Generally open to extending the API, but may want to change the naming a bit. Initial idea for the new API is reloading(every=10, forever=True) instead of reloading(1, reload_after=10) and removing the option to do reloading(True)

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Mar 16, 2021

@julvo thats fine, and definitely not too hard to implement, will take a look today or tomorrow on changing that. i should probably also write some more unit tests for that new behavior.

Generally, the infinite looping definitely isn't required and if that's what's holding up this PR from being merged, i would rather remove it for now and do a second PR with those features.

also add handling of empty reloading in loop
@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Apr 2, 2021

Finally got around to implementing the suggested changes.
I also went ahead and added handling for people using a empty call to reloading() in their loop - will now raise a proper error instead of partial something.

@julvo
Copy link
Copy Markdown
Owner

julvo commented Apr 13, 2021

Hey @laundmo, had a look at some of the patch last weekend. So far only the reloading decorator code - good job! I like the idea of filtering out the reloading decorator instead of remembering the inner function explicitly. Just wanted to let you know that I'm not forgetting to review this, just quite busy at the moment

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Apr 13, 2021 via email

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented May 29, 2021

more than a month later, i already have quite a few people installing reloading directly from my fork, so i might just re-release under a new name because this is taking entirely too long. Would also allow me to do some more drastic changes.

@julvo julvo merged commit eb356d9 into julvo:master Jun 1, 2021
@julvo
Copy link
Copy Markdown
Owner

julvo commented Jun 1, 2021

Just merged this. Sorry again for taking so long - it has been a busy period for me. Of course, feel free to maintain your own fork or also open issues on this repo

closes #10

@laundmo
Copy link
Copy Markdown
Contributor Author

laundmo commented Jun 1, 2021

thanks, i created some issues to keep track of things that need doing, mostly so i wont forget

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.

2 participants