Skip to content
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

feat: live reloading when running as a module #858

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

VishnuSanal
Copy link
Contributor

@VishnuSanal VishnuSanal commented Jun 17, 2024

Description

This PR fixes #654

how to run: python3 -m app --dev

Copy link

vercel bot commented Jun 17, 2024

@VishnuSanal is attempting to deploy a commit to the sparckles Team on Vercel.

A member of the Team first needs to authorize it.

@VishnuSanal VishnuSanal changed the title fixes live reloading when running as a module fix: live reloading when running as a module Jun 17, 2024
Copy link

codspeed-hq bot commented Jun 17, 2024

CodSpeed Performance Report

Merging #858 will not alter performance

Comparing VishnuSanal:module-hot-reload (a7272e5) with main (1e33769)

Summary

✅ 109 untouched benchmarks

@sansyrox
Copy link
Member

sansyrox commented Jun 17, 2024

Hey @VishnuSanal 👋

I am unable to understand the changes. Could you add an example on why we need the new parameters?

And how you'd invoke the changes?

robyn/argument_parser.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @VishnuSanal 👋

I am unable to understand the changes. Could you add an example on why we need the new parameters?

And how you'd invoke the changes?

Hey @VishnuSanal ,
Could you please update the PR description with the above??

@VishnuSanal
Copy link
Contributor Author

uh, oh. my previous comment never got sent.

Could you add an example on why we need the new parameters?

I have changed the implementation, PTAL. removed the need for new params.

And how you'd invoke the changes?

running as a module usually, like python3 -m app --dev would do.

please CMIIW.

@sansyrox
Copy link
Member

sansyrox commented Jun 19, 2024

@VishnuSanal , I just saw your description. That is not what we want to solve for 😄

We want to allow the dev mode to work like robyn -m src --dev. The robyn_app in description is src here

where src

src/
  - __init__.py
  - __main__.py

The --dev flag should rerender based on app changes.

@VishnuSanal VishnuSanal marked this pull request as draft June 19, 2024 13:41
@VishnuSanal VishnuSanal marked this pull request as ready for review June 19, 2024 13:45
@VishnuSanal
Copy link
Contributor Author

thank you for the review, Sanskar! this currently works as expected in my testing, PTAL & LMK what you think. :)

robyn/reloader.py Outdated Show resolved Hide resolved
robyn/reloader.py Outdated Show resolved Hide resolved
robyn/reloader.py Outdated Show resolved Hide resolved
robyn/reloader.py Outdated Show resolved Hide resolved
robyn/reloader.py Outdated Show resolved Hide resolved
robyn/__init__.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @VishnuSanal 👋

Thanks for the PR, I have some comments 😄

robyn/__init__.py Outdated Show resolved Hide resolved
robyn/cli.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @VishnuSanal 👋

Thanks for the update. I have some follow up comments

@VishnuSanal VishnuSanal requested a review from sansyrox June 23, 2024 02:10
robyn/cli.py Outdated Show resolved Hide resolved
env=new_env,
start_new_session=False,
)
if self.config.dev and self.config.file_path is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use self.file_path?

Then we don't need to pass in config.

Copy link
Contributor Author

@VishnuSanal VishnuSanal Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need config.dev. should I pass it as a variable?

edit: we also need running_as_module now.

@VishnuSanal
Copy link
Contributor Author

VishnuSanal commented Jun 26, 2024

current issues:

  • doesn't work when using robyn -m src --dev. sys.argv[0] returns the path to the executable. but, from here, it seems like we shouldn't run it like it, but like python3 -m src --dev.
  • doesn't work during python3 app.py --dev (since path is not present in unknown_args in arguement_parser)
  • doesn't work during robyn app.py --dev

(will edit & update this comment as I fix them...)

@VishnuSanal VishnuSanal changed the title fix: live reloading when running as a module feat: live reloading when running as a module Jul 3, 2024
@VishnuSanal
Copy link
Contributor Author

VishnuSanal commented Jul 3, 2024

note to self:

the flow of control is like: cli#run => cli#start_dev_server => reloader#setup_reloader => EventHandler#reload => Robyn#start and, we are getting the file object at the last stage. my current implementation starts a new dev server from there.

we need a way to bypass it the first time (inorder to reach robyn#start) and, then kill the previous reloader & start new one from there.

as of now, I am getting an infinite loop! will address. :)

@VishnuSanal VishnuSanal deleted the module-hot-reload branch August 22, 2024 06:04
@VishnuSanal VishnuSanal restored the module-hot-reload branch August 24, 2024 06:23
@VishnuSanal VishnuSanal reopened this Aug 24, 2024
@sansyrox sansyrox force-pushed the main branch 5 times, most recently from 07f28de to 0b766c9 Compare January 7, 2025 01:44
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.

Live reloading doesn't work when running as a module
2 participants