Skip to content

Peer Instruction Migration to FastAPI#1209

Merged
bnmnetp merged 18 commits into
RunestoneInteractive:mainfrom
sethbern:peer-fastapi
May 27, 2026
Merged

Peer Instruction Migration to FastAPI#1209
bnmnetp merged 18 commits into
RunestoneInteractive:mainfrom
sethbern:peer-fastapi

Conversation

@sethbern

@sethbern sethbern commented May 7, 2026

Copy link
Copy Markdown
Contributor

Hey @bnmnetp, I have done a first pass at moving peer instruction over to FastAPI. For now I am focusing on sync PI to get things working end-to-end before touching async.

Sync PI routes are in, Jinja2 templates ported from the old web2py views, and nav links fixed across _base.html, menu.html, and the Sphinx book theme so everything points to /assignment/peer/.... Async stubs are there but not wired up yet. I've tested locally and seems to be working ok.

A couple things I wanted your eyes on before I keep going:

  1. LTI score sending, I am not super familiar with this part of the codebase so I followed the existing pattern. Does the approach look right?
  2. Overall structure good to go before I move on to async or is there anything else that seems wrong?

@bnmnetp

bnmnetp commented May 7, 2026

Copy link
Copy Markdown
Member

Thanks @sethbern . It looks like you found and cleaned up a number of things in my previous work on porting. The overall structure looks good so keep going I say.

I'll take a closer look at the LTI stuff. I'm just back from two weeks of travel, so I've got a number of other things to take care of before I'm fully back at my desk.

@bnmnetp

bnmnetp commented May 11, 2026

Copy link
Copy Markdown
Member

If you could install black and run it to reformat peer.py so it passes the lint tests that would be great.

@sethbern

Copy link
Copy Markdown
Contributor Author

@bnmnetp @barbarer The web2py peer instructor dashboard has a nav bar with tabs for Student Progress, Admin, Grading, Assignments, Practice, and Help/Documentation. Should this nav bar be included in the new FastAPI peer interface at all? And if so, for the tabs that don't have FastAPI routes yet (Student Progress, Practice, Help/Documentation), should I create them, or are they not worth porting?
image

@bnmnetp

bnmnetp commented May 14, 2026

Copy link
Copy Markdown
Member

No, you can leave the nav bar tabs out of the new FastAPI interface.

@sethbern

Copy link
Copy Markdown
Contributor Author

Hey @bnmnetp, I've migrated both synchronous and asynchronous peer instruction to FastAPI and tested locally. I think this is ready for review.

@sethbern sethbern marked this pull request as ready for review May 20, 2026 14:02
@sethbern sethbern requested a review from bnmnetp as a code owner May 20, 2026 14:02
@bnmnetp

bnmnetp commented May 20, 2026

Copy link
Copy Markdown
Member

Great, I'll take a look. The crud_tests are passing locally for me, so I'm curious if you've looked into why they are failing for this PR?

I'll plan to merge soon so that we can run this in parallel with the old until we have tested it a bit more in the wild.

@sethbern

Copy link
Copy Markdown
Contributor Author

I did look into it but couldn't fully figure it out. It seems like CI might be picking up an old version of rsptx.db.crud but I'm not totally sure.

@bnmnetp

bnmnetp commented May 20, 2026

Copy link
Copy Markdown
Member

have you fetched origin/main and rebased?

@bnmnetp

bnmnetp commented May 20, 2026

Copy link
Copy Markdown
Member

CI creates a whole new virtual machine and environment each time it runs so there are no caching issues possible.

@sethbern

Copy link
Copy Markdown
Contributor Author

I just rebased and it is still not passing. I can dig into it a bit more to see whats causing the issue

@bnmnetp

bnmnetp commented May 20, 2026

Copy link
Copy Markdown
Member

I'm sure you know this, but you can run the tests locally too.

@bnmnetp

bnmnetp commented May 20, 2026

Copy link
Copy Markdown
Member

Yikes what happened? This PR now has 423 files changed!!??

@sethbern

Copy link
Copy Markdown
Contributor Author

I accidentally rebased onto origin/main instead of upstream/main, which is why all those extra commits showed up. When I run the tests locally they're all passing on my end

@sethbern

Copy link
Copy Markdown
Contributor Author

@bnmnetp Tests are passing now

@bnmnetp

bnmnetp commented May 26, 2026

Copy link
Copy Markdown
Member

Yes, I fixed the import problem, and it looks like you have rebased this back to a manageable size. I'll review and try to get this merged in the next couple days.

@bnmnetp

bnmnetp commented May 26, 2026

Copy link
Copy Markdown
Member

For posterity:

There were problems importing anything from the crud package that were added after I branched runestone 7.13.5. As rsmanage was installing 7.13.5 (whereas before the branch it always used the development version of runestone rather than released.). 7.13.5 is the last released version of runestone that supports building a book written in RST. runestone 8.x only supports the javascript and css for including components in books authored in PreTeXt.

@bnmnetp

bnmnetp commented May 26, 2026

Copy link
Copy Markdown
Member

Thanks, could you please update the documentation to keep the JSON schema up to date for questions. See question_json_schema.rst. Its important to keep this up to date as others are working on providing questions to us in JSON and I want to make sure this document stays current and accurate.

Its chapter 19 in https://docs.runestone.academy

@bnmnetp bnmnetp merged commit c1468fc into RunestoneInteractive:main May 27, 2026
2 checks passed
Integer
) # only reading assignments will have this populated
use_llm = Column(Web2PyBoolean, server_default="F")
async_mode = Column(String(20), server_default="standard")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI -- This should have been in a migration. It brought down the assignment page in production until I added it this morning.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In fact I think I got the migration wrong as I didn't remove the use_llm field. Totally fixable.

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