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: ux behavior on save for anonymous users #453

Merged
merged 18 commits into from
Oct 7, 2024
Merged

Conversation

nikochiko
Copy link
Member

@nikochiko nikochiko commented Aug 30, 2024

should redirect to login page with next=...

Q/A checklist

  • If you add new dependencies, did you update the lock file?
poetry lock --no-update
  • Run tests
ulimit -n unlimited && ./scripts/run-tests.sh
  • Do a self code review of the changes - Read the diff at least twice.
  • Carefully think about the stuff that might break because of this change - this sounds obvious but it's easy to forget to do "Go to references" on each function you're changing and see if it's used in a way you didn't expect.
  • The relevant pages still run when you press submit
  • The API for those pages still work (API tab)
  • The public API interface doesn't change if you didn't want it to (check API tab > docs page)
  • Do your UI changes (if applicable) look acceptable on mobile?
  • Ensure you have not regressed the import time unless you have a good reason to do so.
    You can visualize this using tuna:
python3 -X importtime -c 'import server' 2> out.log && tuna out.log

To measure import time for a specific library:

$ time python -c 'import pandas'

________________________________________________________
Executed in    1.15 secs    fish           external
   usr time    2.22 secs   86.00 micros    2.22 secs
   sys time    0.72 secs  613.00 micros    0.72 secs

To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:

def my_function():
    import pandas as pd
    ...

Legal Boilerplate

Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.


@devxpy devxpy self-assigned this Sep 14, 2024
@devxpy
Copy link
Member

devxpy commented Sep 14, 2024

curious if this is upto date as per

@SeanBlagsvedt This is the intended UX flow for anonymous users. https://docs.google.com/presentation/d/1P5MKF0jykYPw-G_9SgvKEpu7fjD2v-0gZgLJB4Uw174/edit#slide=id.g2ef5e525406_0_329

daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
@devxpy devxpy assigned nikochiko and unassigned devxpy Sep 14, 2024
@nikochiko
Copy link
Member Author

curious if this is upto date as per

@SeanBlagsvedt This is the intended UX flow for anonymous users. https://docs.google.com/presentation/d/1P5MKF0jykYPw-G_9SgvKEpu7fjD2v-0gZgLJB4Uw174/edit#slide=id.g2ef5e525406_0_329

needs an update, if I remember right

@nikochiko nikochiko assigned devxpy and unassigned nikochiko Sep 18, 2024
return

try:
request_state = base64.urlsafe_b64decode(request_state).decode()
Copy link
Member

Choose a reason for hiding this comment

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

Not a good idea. Better to save the state in db but not actually call the celery task.

Copy link
Member Author

Choose a reason for hiding this comment

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

but for anon users, we won't have their real uid until they login.

are you suggesting that we create a run without a uid, and when the user comes back with SUBMIT_AFTER_LOGIN_Q, we then update the uid and run?

Copy link
Member

Choose a reason for hiding this comment

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

we generate a firebase user as soon as you submit, this should do the same thing

Copy link
Member Author

Choose a reason for hiding this comment

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

for anon users who come back with an existing account, we would have 2 different uids.

i fixed this case by setting the uid of the unqueued run when user comes back. i added a concurrency check there too so that we don't queue the same run twice.

daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
daras_ai_v2/base.py Outdated Show resolved Hide resolved
@devxpy devxpy assigned nikochiko and unassigned devxpy Sep 18, 2024
@nikochiko nikochiko assigned devxpy and unassigned nikochiko Sep 19, 2024
daras_ai_v2/base.py Outdated Show resolved Hide resolved
@nikochiko
Copy link
Member Author

That sounds like a plausible user flow. I'll make it such that the Copy Link button also saves like the other Save button

this would require more work than I had thought. right now it doesn't trigger a form submit - all javascript.

@nikochiko
Copy link
Member Author

fixed the rest, other than the Copy Link fix.

i'll need to release gooey-gui and update version for that.

@devxpy
Copy link
Member

devxpy commented Sep 23, 2024

That sounds like a plausible user flow. I'll make it such that the Copy Link button also saves like the other Save button

this would require more work than I had thought. right now it doesn't trigger a form submit - all javascript.

You can execute js on a python button I believe

if button_pressed:
   gui.js(<js code here>)

@devxpy devxpy assigned nikochiko and unassigned devxpy Sep 23, 2024
@nikochiko nikochiko assigned devxpy and unassigned nikochiko Sep 23, 2024
@nikochiko
Copy link
Member Author

done

daras_ai_v2/base.py Outdated Show resolved Hide resolved
@nikochiko nikochiko merged commit b89383a into master Oct 7, 2024
6 checks passed
@nikochiko nikochiko deleted the save-for-anon-users branch October 7, 2024 05:49
@devxpy
Copy link
Member

devxpy commented Oct 7, 2024

I get this error when I try to press the options button as a fresh anonymous user

Traceback (most recent call last):
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/uvicorn/protocols/http/httptools_impl.py", line 404, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/fastapi/applications.py", line 270, in __call__
    await super().__call__(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/applications.py", line 124, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/Users/dev/Projects/dara/ddgai/./server.py", line 107, in middleware
    await app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/sessions.py", line 86, in __call__
    await self.app(scope, receive, send_wrapper)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/authentication.py", line 48, in __call__
    await self.app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/cors.py", line 92, in __call__
    await self.simple_response(scope, receive, send, request_headers=headers)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/cors.py", line 147, in simple_response
    await self.app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 75, in __call__
    raise exc
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 64, in __call__
    await self.app(scope, receive, sender)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/routing.py", line 680, in __call__
    await route.handle(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/routing.py", line 275, in handle
    await self.app(scope, receive, send)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/routing.py", line 65, in app
    response = await func(request)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/fastapi/routing.py", line 231, in app
    raw_response = await run_endpoint_function(
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/fastapi/routing.py", line 162, in run_endpoint_function
    return await run_in_threadpool(dependant.call, **values)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/starlette/concurrency.py", line 41, in run_in_threadpool
    return await anyio.to_thread.run_sync(func, *args)
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/anyio/to_thread.py", line 33, in run_sync
    return await get_asynclib().run_sync_in_worker_thread(
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 877, in run_sync_in_worker_thread
    return await future
  File "/Users/dev/.virtualenvs/ddgai-b0692ca2/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 807, in run
    result = context.run(func, *args)
  File "/Users/dev/Projects/dara/gooey-gui/py/gooey_gui/core/renderer.py", line 70, in wrapper
    return renderer(
  File "/Users/dev/Projects/dara/gooey-gui/py/gooey_gui/core/renderer.py", line 117, in renderer
    ret = render()
  File "/Users/dev/Projects/dara/ddgai/routers/root.py", line 615, in recipe_or_handle_or_static
    return render_recipe_page(request, parts[0], RecipeTabs.run, example_id)
  File "/Users/dev/Projects/dara/ddgai/routers/root.py", line 691, in render_recipe_page
    page.render()
  File "/Users/dev/Projects/dara/ddgai/./daras_ai_v2/base.py", line 362, in render
    self._render_header()
  File "/Users/dev/Projects/dara/ddgai/./daras_ai_v2/base.py", line 408, in _render_header
    self._render_options_button_with_dialog()
  File "/Users/dev/Projects/dara/ddgai/./daras_ai_v2/base.py", line 473, in _render_options_button_with_dialog
    self._unsaved_options_modal()
  File "/Users/dev/Projects/dara/ddgai/./daras_ai_v2/base.py", line 844, in _unsaved_options_modal
    assert self.request.user
AssertionError

).update(run_status=STARTING_STATE, uid=self.request.user.uid)
if updated_count >= 1:
# updated now
self.dump_state_to_sr(self._get_validated_state(), self.current_sr)
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this is needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, good catch. i think it is leftover from the earlier hack where the state was carried over without creating a saved run. thanks!

@nikochiko nikochiko restored the save-for-anon-users branch October 7, 2024 10:21
@nikochiko
Copy link
Member Author

@devxpy fixed both in #481

@devxpy
Copy link
Member

devxpy commented Oct 8, 2024

I can still repro this in prod:

  • go to /copilot
  • make an edit to prompt
  • press save and run
  • login
  • run stuck on starting

@nikochiko
Copy link
Member Author

fixed in #482 with self.current_sr.refresh_from_db() before queueing.

cause was that we updated current_sr.uid in the database with SavedRun.objects.update(), but not in the ORM object. we should probably get rid of uid from the runner task too. SavedRun.id should be good enough

@devxpy
Copy link
Member

devxpy commented Oct 8, 2024

this also broke rate limits fixed in 7fe01cb

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