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

Add support for FastAPI (Starlette) #115

Closed
1 of 3 tasks
orlovmyk opened this issue Mar 17, 2023 · 3 comments
Closed
1 of 3 tasks

Add support for FastAPI (Starlette) #115

orlovmyk opened this issue Mar 17, 2023 · 3 comments

Comments

@orlovmyk
Copy link

orlovmyk commented Mar 17, 2023

As discussed in this PR #47

  • Support dropped

Current library still supports Python 2.7 and 3.5 but Starlette requires Python 3.6+. I'm going to remove support of these versions in some future.

  • FastAPI becomes more and more popular, I can agree that library right now is not suitable for FastAPI, but it doesn't mean that support is not needed. It's not about FastAPI, more like any async web framework. I understand that having mix of async/non-async functions can be nightmare, but it's possible ti use python entry point to separate them. Something like pip install pylti1p3[async].

At the current moment I don't consider FastAPI as a suitable framework for the LTI interaction within the current implementation on the library engine. I understand that FastAPI supports async and non-async operations (and as i see from your example you've used it in non-async way) but in my opinion the main advantages of the FastAPI usage in async views. For now the current library contains some http calls (using requests library) + non-async cache usage so it can't be used in case of asynchronous views. So in my opinion first of all this library should be refactored to support usage from the async functions.

  • It depends, because if library itself will use async operations, there won't be any problem with handling POST data. Also you can do await call with getting POST data before sending it to function (non-async way)

At the current moment I don't consider FastAPI as a suitable framework for the LTI interaction within the current implementation on the library engine. I understand that FastAPI supports async and non-async operations (and as i see from your example you've used it in non-async way) but in my opinion the main advantages of the FastAPI usage in async views. For now the current library contains some http calls (using requests library) + non-async cache usage so it can't be used in case of asynchronous views. So in my opinion first of all this library should be refactored to support usage from the async functions.

If someone is interested in this too, this .tgz gives high-level idea on how this can be achieved
lti_advantage.tar.gz

@FeldrinH
Copy link

I'm also looking into adding async support but for aiohttp server instead of Starlette.

@dmitry-viskov You noted that the current implementation contains "non-async cache usage":

For now the current library contains some http calls (using requests library) + non-async cache usage so it can't be used in case of asynchronous views.

Could you clarify what you meant by non-async cache usage? I took a quick look at the pylti1.3 code and could not find any cache usage that would be incompatible with async.

@dmitry-viskov
Copy link
Owner

dmitry-viskov commented Mar 10, 2024

hi @FeldrinH.

Could you clarify what you meant by non-async cache usage? I took a quick look at the pylti1.3 code and could not find any cache usage that would be incompatible with async.

  1. This library uses requests library for HTTP-communications (it isn't async).

  2. This library originally was created for Django/Flask so it uses built-in caching mechanisms of this non-async frameworks. Example: https://github.com/dmitry-viskov/pylti1.3-flask-example/blob/master/game/app.py#L72

  3. I don't see any point in supporting async in this library. FastAPI is oriented primarily on creating REST APIs. LTI is very specific protocol that is based on templates/html/cookies and other things. All this is not about REST API. Also i believe that build projects using asyncio/etc specially for LTI - 100% bad idea.

  4. As i mentioned previously LTI as protocol couldn't be implemented separately from web framework (because it uses request/response objects, built-in mechanisms for redirects and etc). Current library provides some abstractions that could be used for implementing LTI using your own framework but anyway I don't want to support all this custom adapters in scope of this library. Current adapters for Django/Flask is enough. If you want to support asyncio or other things you may just create fork or create your own adapter as the separate project

@FeldrinH
Copy link

Thank you for the response!

I'm not sure I understand why using an async web framework/asyncio for LTI would be a bad idea, but since this library won't be getting new features in the foreseeable future, there probably isn't too much value in debating the merits of async here.

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

3 participants