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

Reconsider how we use/advertise idom.run() #657

Closed
3 tasks
Archmonger opened this issue Feb 8, 2022 · 23 comments · Fixed by #703
Closed
3 tasks

Reconsider how we use/advertise idom.run() #657

Archmonger opened this issue Feb 8, 2022 · 23 comments · Fixed by #703
Labels
priority-3-low May be resolved one any timeline. type-docs About changes and updates to documentation type-revision About a change in functionality or behavior
Milestone

Comments

@Archmonger
Copy link
Contributor

Archmonger commented Feb 8, 2022

Current Situation

idom.run() probably shouldn't be the primary advertised way of using IDOM. Especially in it's current state.

Using a development webserver for production use is a big no-no, and by prominently displaying idom.run() as the first steps to using IDOM shows we are encouraging people to do just that. I believe the primary way of utilizing IDOM should be embedding within existing applications.

Proposed Actions

  • Keep idom.run() as is, but add logging to indicate idom.run() is not suited for production use.
    • ex. idom.run() is only intended for testing purposes. For production use, consider embedding IDOM within an existing project. Check out the docs for more info.
  • Expand the ease-of-use for integrating into supported web frameworks, similar to Django-IDOM
  • Remove SharedClientState, as it is not compatible with multiprocessed webservers.
    • This is also an opportunity to rename PerClientState to IdomView

Optional

  • Change the callable name/path to more appropriately indicate danger or for testing purposes only
    • idom.testing.webserver()
    • idom.shortcuts.run()
    • idom.devtools.run_webserver()
  • Remove the pip parameter for frameworks (it's really not needed)
    • idom.run() should raise a simple to understand exception if the user has forgotten to install a web framework
    • ex. Looks like you don't have a web framework installed. You'll need to pip install one of the following: fastapi, flask, sanic
@Archmonger Archmonger added the flag-triage Not prioritized. label Feb 8, 2022
@Archmonger
Copy link
Contributor Author

Archmonger commented Feb 8, 2022

My personal opinion is that idom.run() should either be removed or tucked away into a "dev tools" closet as-is and shouldn't be prominently advertised.

Instead, we should focus on simplifying how users integrate IDOM into existing applications. It's an over-reach of scope for us to handle anything related to launching a WSGI/ASGI webserver.

@rmorshea rmorshea added priority-2-moderate Should be resolved on a reasonable timeline. priority-3-low May be resolved one any timeline. type-revision About a change in functionality or behavior labels Feb 8, 2022
@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

We could add a big fat warning about this instead? Technically doing idom.run is fine with Sanic (and I think Tornado too), but not the others.

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

Meh, nevermind. We should completely shift how this works:

  • Drop the run function
  • stop calling things a per-client state "server" and instead call it a "view"
  • Make app a required argument of PerClientStateView etc.
  • Rename idom.server.proto.ServerType.run to run_dev_server

Doing this with appropriate deprecation warnings will be a bit tricky, but I think that sends a more accurate message about how you should expect to use IDOM.

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

The only question then is how you many the "hello world" still look simple...

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

Maybe do:

from idom import component, html, server


@component
def App():
    return html.h1("Hello, World!")


server.default.run(App)

Except run only accepts one argument (use lack of configuration as deterrent) and produces a big fat warning.

or maybe server.dev.run?

@rmorshea rmorshea removed flag-triage Not prioritized. priority-3-low May be resolved one any timeline. labels Feb 8, 2022
@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

There's also a question of the install target: #600

I think we should we remove any sense of a "default" install target too since you'd always need to explicitly call out which server implementation you're using.

There's no way to give a deprecation warning for this removal though as far as I can tell unless pip knows to find the last version with a given install target.

@Archmonger
Copy link
Contributor Author

Archmonger commented Feb 8, 2022

Doing this with appropriate deprecation warnings will be a bit tricky, but I think that sends a more accurate message about how you should expect to use IDOM.

IDOM is still <1.0.0, technically we've never needed to deprecate anything. Immediate breaking changes are fully acceptable on pre-release version numbers.

The only question then is how you many the "hello world" still look simple...

Maybe we consider a pattern like this (with the run callable name being TBD)

from idom import component, html, devtools
from fastapi import FastAPI

fastapi_app = FastAPI()

@component
def Hello():
    return html.h1("Hello, World!")

# This should print a warning that it is not production usable
devtools.run(fastapi_app, component=Hello)

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

I'm worried about the hello-world implying a preference for a given server implementation.

@Archmonger
Copy link
Contributor Author

Archmonger commented Feb 8, 2022

That's pretty much an impossible problem to solve without multiple examples for each framework. We support multiple web frameworks, but we need to choose at least one to first demo with.

Could just be a comment saying "Utilize whatever Python web framework you like here!"

@Archmonger
Copy link
Contributor Author

  • stop calling things a per-client state "server" and instead call it a "view"
  • Make app a required argument of PerClientStateView etc.
  • Rename idom.server.proto.ServerType.run to run_dev_server

That's a good first step. This actually brings up another thing I've forgotten to mention. I really dislike the name PerClientState<X>. It's really hard to tell what it does based on that name.

@Archmonger
Copy link
Contributor Author

I think we should we remove any sense of a "default" install target too since you'd always need to explicitly call out which server implementation you're using.

I also agree with this. It's best for us to not dictate what gets used.

In that case, we really don't need a parameter at all. We can just say it's up to them to separately install a web framework, similar to how whitenoise has no Flask/FastAPI/Django/etc pip parameters,

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

In that case, we really don't need a parameter at all.

True

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

I really dislike the name PerClientState

I've gotten an inverse comment at some point. With that said, I had a hard time coming up with a name that accurately described what they did so I'd be open to suggestions.

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 8, 2022

IDOM is still <1.0.0, technically we've never needed to deprecate anything.

I feel some obligation to make things easier on early adopters where possible.

@Archmonger
Copy link
Contributor Author

Archmonger commented Feb 8, 2022

Based on your your thoughts above, I think I've come up with the best solution for everyone.

Implement the following

  1. Keep idom.run() as is, but add logging to indicate idom.run() is not suited for production use.
    • ex. idom.run() is only intended for testing purposes. For production use, consider embedding IDOM within an existing project. Check out the docs for more info.
  2. Expand the ease-of-use for integrating into supported web frameworks, similar to Django-IDOM

Optional

  • Change the callable name/path to more appropriately indicate danger or for testing purposes only
  • Remove the pip parameter for frameworks (it's really not needed)
  • idom.run() should raise a simple to understand exception if the user has forgotten to install a web framework
    • ex. Looks like you don't have a web framework installed. You'll need to pip install one of the following: fastapi, flask, sanic

@Archmonger
Copy link
Contributor Author

Archmonger commented Feb 8, 2022

I really dislike the name PerClientState

I've gotten an inverse comment at some point. With that said, I had a hard time coming up with a name that accurately described what they did so I'd be open to suggestions.

Just removing State from the name makes it a lot easier to understand. For example PerClientView and SharedView.

On the topic of SharedView

The multiprocessing issue with SharedView will become an even bigger problem as we start recommending people to use actual webservers to serve IDOM. I'm sure the majority of people use multiple worker processes in any realistic deployment scenario.

I'd honestly just recommend removing it now. As far as I know, it's not possible to obtain a multiprocessing safe SharedView. Especially in the context of a webserver's ASGI worker processes, since we don't have direct access to the worker process callable.

Here's what I think all our options are

  • Remove SharedView and have shared states to be left up to the user
  • Remove SharedView and create framework-specific shared state hooks when technologically possible
  • Have redis become a requirement for SharedView

If we completely remove SharedView, then we also gain the luxury of renaming PerClientView to something simple like IdomView.

@Archmonger
Copy link
Contributor Author

On a side note, I've decided to commit to creating IDOM video tutorials. We're going to need something for our visual learners to increase adoption.

I'll try and make that happen sometime this year.

@Archmonger Archmonger added the type-docs About changes and updates to documentation label Feb 9, 2022
@Archmonger
Copy link
Contributor Author

Let me know if you agree on #657 (comment)

@Archmonger Archmonger added priority-3-low May be resolved one any timeline. and removed priority-2-moderate Should be resolved on a reasonable timeline. labels Feb 9, 2022
@Archmonger Archmonger added this to the 1.0 milestone Feb 16, 2022
@rmorshea
Copy link
Collaborator

I think this issue may be related: #653

@rmorshea rmorshea reopened this Feb 17, 2022
@rmorshea
Copy link
Collaborator

Ah, whoops, didn't read through your comment.

Yes, I think I agree.

@rmorshea
Copy link
Collaborator

Probably with the addition that idom.run takes a single argument (the component) to discourage use in the long term.

@Archmonger
Copy link
Contributor Author

How do you feel about removing SharedView? #657 (comment)

@rmorshea
Copy link
Collaborator

I agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low May be resolved one any timeline. type-docs About changes and updates to documentation type-revision About a change in functionality or behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants