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

fix: various improvements around the dev flag #388

Merged
merged 3 commits into from
Feb 11, 2023

Conversation

AntoineRR
Copy link
Collaborator

Description

This PR fixes #385

Sorry, there are a lot of changes here 😅 I started working on the issue mentioned above and figured a lot could be improved on the way. This includes:

  • dev flag launching the app almost in the same way as without the flag. This means we should be able to run multiple workers and processes with the dev flag, and get the logs based on the specified log-level flag correctly!
  • ArgumentParser becomes Config, and this config is an attribute of the Robyn class. I think it is a good idea to store all the configurations from the user in a single object.
  • A new Logger class which is just a helper for logging nice messages with colors.
  • Moved the process starting methods to processpool.py to use them both in the Robyn.start method and the dev_event_handler.py file

That was hard to debug, but from my personal tests it should work fine. @sansyrox if you have any ideas for writing unit tests, I could add some here!

@netlify
Copy link

netlify bot commented Feb 2, 2023

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit f875ef0
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63e6c7c8a21fe300086275a3

@AntoineRR AntoineRR requested a review from sansyrox February 3, 2023 22:16
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Hey @AntoineRR 👋

Thank you for your PR 😄

I have had a first look on the PR but I will do a thorough review tomorrow 😄

@sansyrox
Copy link
Member

sansyrox commented Feb 6, 2023

@AntoineRR ,

. @sansyrox if you have any ideas for writing unit tests, I could add some here!

I don't like unit tests. I know it is a controversial opinion but 🤷 We can look into improving the integration tests if really need be.

robyn/argument_parser.py Outdated Show resolved Hide resolved
robyn/logger.py Outdated Show resolved Hide resolved
src/server.rs Show resolved Hide resolved
@sansyrox
Copy link
Member

sansyrox commented Feb 6, 2023

@AntoineRR , thanks for you PR 😄 I have a few comments. And apologies for the late review

@AntoineRR
Copy link
Collaborator Author

Thanks for the review @sansyrox, and don't worry about the timing 🙂 I pushed a new commit based on your comments. Regarding tests, I was thinking about integration tests too, but don't know how to deal with it and what to test for the dev flag.
Btw the tests are failing on macos for Python 3.8 and above, but pass for 3.7... I don't have a mac to test that but can try to fix it anyway.

@sansyrox
Copy link
Member

sansyrox commented Feb 8, 2023

The PR looks good to me. Not doing a green tick approval as I will check why the tests are failing tomorrow 😅

@AntoineRR
Copy link
Collaborator Author

Thanks a lot if you can take a look at those tests 😄 Let me know if I can help, I'll probably try some things on my side too.

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Hey @AntoineRR ,

I tried running the code on Mac but it doesn't start.

It exits with the following error:

INFO:robyn.logger:Starting server at 127.0.0.1:8080
Traceback (most recent call last):
  File "/Users/sanskar/repos/robyn/integration_tests/base_routes.py", line 496, in <module>
    app.start(port=8080)
  File "/Users/sanskar/repos/robyn/robyn/__init__.py", line 117, in start
    run_processes(
  File "/Users/sanskar/repos/robyn/robyn/processpool.py", line 30, in run_processes
    process_pool = init_processpool(
  File "/Users/sanskar/repos/robyn/robyn/processpool.py", line 100, in init_processpool
    process.start()
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/process.py", line 121, in start
    self._popen = self._Popen(self)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/context.py", line 224, in _Popen
    return _default_context.get_context().Process._Popen(process_obj)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/context.py", line 288, in _Popen
    return Popen(process_obj)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/popen_spawn_posix.py", line 32, in __init__
    super().__init__(process_obj)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/popen_fork.py", line 19, in __init__
    self._launch(process_obj)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/popen_spawn_posix.py", line 47, in _launch
    reduction.dump(process_obj, fp)
  File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/multiprocessing/reduction.py", line 60, in dump
    ForkingPickler(file, protocol).dump(obj)
TypeError: cannot pickle 'builtins.FunctionInfo' object

robyn/processpool.py Outdated Show resolved Hide resolved
@AntoineRR
Copy link
Collaborator Author

multiprocessing was indeed what made the tests fail on macOS, all fixed now. Thanks a lot for your help @sansyrox 😄

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @AntoineRR

@sansyrox sansyrox merged commit ee17878 into sparckles:main Feb 11, 2023
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.

[BUG] The dev flag doesn't set the log level to DEBUG
2 participants