-
Notifications
You must be signed in to change notification settings - Fork 21
Feat rate limit #386
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
base: main
Are you sure you want to change the base?
Feat rate limit #386
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |||||
|
|
||||||
| app = FastAPI() | ||||||
|
|
||||||
|
|
||||||
| def json_serializer(obj): | ||||||
| """JSON serializer for objects not serializable by default json code""" | ||||||
| if isinstance(obj, (datetime.datetime, datetime.date, datetime.time)): | ||||||
|
|
@@ -255,10 +256,16 @@ async def cli_auth(auth_provider: str, code: str, state: str, db_context=Depends | |||||
| raise e | ||||||
| except Exception as e: | ||||||
| # Catch unexpected errors during OAuth handling | ||||||
| raise HTTPException(status_code=500, detail=f"Error during {auth_provider} OAuth flow: {e}") from e | ||||||
| raise HTTPException( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are these actual lint changes or claude gone wild? |
||||||
| status_code=500, | ||||||
| detail=f"Error during {auth_provider} OAuth flow: {e}", | ||||||
| ) from e | ||||||
|
|
||||||
| if not user_id or not user_name: | ||||||
| raise HTTPException(status_code=500,detail="Failed to retrieve user ID or username from provider.",) | ||||||
| raise HTTPException( | ||||||
| status_code=500, | ||||||
| detail="Failed to retrieve user ID or username from provider.", | ||||||
| ) | ||||||
|
|
||||||
| try: | ||||||
| with db_context as db: | ||||||
|
|
@@ -268,7 +275,10 @@ async def cli_auth(auth_provider: str, code: str, state: str, db_context=Depends | |||||
| db.create_user_from_cli(user_id, user_name, cli_id, auth_provider) | ||||||
|
|
||||||
| except AttributeError as e: | ||||||
| raise HTTPException(status_code=500, detail=f"Database interface error during update: {e}") from e | ||||||
| raise HTTPException( | ||||||
| status_code=500, | ||||||
| detail=f"Database interface error during update: {e}", | ||||||
| ) from e | ||||||
| except Exception as e: | ||||||
| raise HTTPException(status_code=400, detail=f"Database update failed: {e}") from e | ||||||
|
|
||||||
|
|
@@ -280,6 +290,7 @@ async def cli_auth(auth_provider: str, code: str, state: str, db_context=Depends | |||||
| "is_reset": is_reset, | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| async def _stream_submission_response( | ||||||
| submission_request: SubmissionRequest, | ||||||
| submission_mode_enum: SubmissionMode, | ||||||
|
|
@@ -298,18 +309,22 @@ async def _stream_submission_response( | |||||
|
|
||||||
| while not task.done(): | ||||||
| elapsed_time = time.time() - start_time | ||||||
| yield f"event: status\ndata: {json.dumps({'status': 'processing', | ||||||
| 'elapsed_time': round(elapsed_time, 2)}, | ||||||
| default=json_serializer)}\n\n" | ||||||
| status_data = json.dumps( | ||||||
| {"status": "processing", "elapsed_time": round(elapsed_time, 2)}, | ||||||
| default=json_serializer, | ||||||
| ) | ||||||
| yield f"event: status\ndata: {status_data}\n\n" | ||||||
|
|
||||||
| try: | ||||||
| await asyncio.wait_for(asyncio.shield(task), timeout=15.0) | ||||||
| except asyncio.TimeoutError: | ||||||
| continue | ||||||
| except asyncio.CancelledError: | ||||||
| yield f"event: error\ndata: {json.dumps( | ||||||
| {'status': 'error', 'detail': 'Submission cancelled'}, | ||||||
| default=json_serializer)}\n\n" | ||||||
| error_data = json.dumps( | ||||||
| {"status": "error", "detail": "Submission cancelled"}, | ||||||
| default=json_serializer, | ||||||
| ) | ||||||
| yield f"event: error\ndata: {error_data}\n\n" | ||||||
| return | ||||||
|
|
||||||
| result, reports = await task | ||||||
|
|
@@ -343,6 +358,7 @@ async def _stream_submission_response( | |||||
| except asyncio.CancelledError: | ||||||
| pass | ||||||
|
|
||||||
|
|
||||||
| @app.post("/{leaderboard_name}/{gpu_type}/{submission_mode}") | ||||||
| async def run_submission( # noqa: C901 | ||||||
| leaderboard_name: str, | ||||||
|
|
@@ -381,13 +397,13 @@ async def run_submission( # noqa: C901 | |||||
| ) | ||||||
| return StreamingResponse(generator, media_type="text/event-stream") | ||||||
|
|
||||||
|
|
||||||
| async def enqueue_background_job( | ||||||
| req: ProcessedSubmissionRequest, | ||||||
| mode: SubmissionMode, | ||||||
| backend: KernelBackend, | ||||||
| manager: BackgroundSubmissionManager, | ||||||
| ): | ||||||
|
|
||||||
| # pre-create the submission for api returns | ||||||
| with backend.db as db: | ||||||
| sub_id = db.create_submission( | ||||||
|
|
@@ -401,7 +417,8 @@ async def enqueue_background_job( | |||||
| job_id = db.upsert_submission_job_status(sub_id, "initial", None) | ||||||
| # put submission request in queue | ||||||
| await manager.enqueue(req, mode, sub_id) | ||||||
| return sub_id,job_id | ||||||
| return sub_id, job_id | ||||||
|
|
||||||
|
|
||||||
| @app.post("/submission/{leaderboard_name}/{gpu_type}/{submission_mode}") | ||||||
| async def run_submission_async( | ||||||
|
|
@@ -425,37 +442,49 @@ async def run_submission_async( | |||||
| Raises: | ||||||
| HTTPException: If the kernelbot is not initialized, or header/input is invalid. | ||||||
| Returns: | ||||||
| JSONResponse: A JSON response containing job_id and and submission_id for the client to poll for status. | ||||||
| JSONResponse: A JSON response containing job_id and submission_id. | ||||||
| The client can poll for status using these ids. | ||||||
| """ | ||||||
| try: | ||||||
|
|
||||||
| await simple_rate_limit() | ||||||
| logger.info(f"Received submission request for {leaderboard_name} {gpu_type} {submission_mode}") | ||||||
|
|
||||||
| logger.info( | ||||||
| "Received submission request for %s %s %s", | ||||||
| leaderboard_name, | ||||||
| gpu_type, | ||||||
| submission_mode, | ||||||
| ) | ||||||
|
|
||||||
| # throw error if submission request is invalid | ||||||
| try: | ||||||
| submission_request, submission_mode_enum = await to_submit_info( | ||||||
| user_info, submission_mode, file, leaderboard_name, gpu_type, db_context | ||||||
| user_info, submission_mode, file, leaderboard_name, gpu_type, db_context | ||||||
| ) | ||||||
|
|
||||||
| req = prepare_submission(submission_request, backend_instance) | ||||||
|
|
||||||
| except KernelBotError as e: | ||||||
| raise HTTPException(status_code=e.http_code, detail=str(e)) from e | ||||||
| except Exception as e: | ||||||
| raise HTTPException(status_code=400, detail=f"failed to prepare submission request: {str(e)}") from e | ||||||
| raise HTTPException( | ||||||
| status_code=400, | ||||||
| detail=f"failed to prepare submission request: {str(e)}", | ||||||
| ) from e | ||||||
|
Comment on lines
+465
to
+471
|
||||||
|
|
||||||
| # prepare submission request before the submission is started | ||||||
| if not req.gpus or len(req.gpus) != 1: | ||||||
| raise HTTPException(status_code=400, detail="Invalid GPU type") | ||||||
|
|
||||||
| # put submission request to background manager to run in background | ||||||
| sub_id,job_status_id = await enqueue_background_job( | ||||||
| sub_id, job_status_id = await enqueue_background_job( | ||||||
| req, submission_mode_enum, backend_instance, background_submission_manager | ||||||
| ) | ||||||
|
|
||||||
| return JSONResponse( | ||||||
| status_code=202, | ||||||
| content={"details":{"id": sub_id, "job_status_id": job_status_id}, "status": "accepted"}, | ||||||
| content={ | ||||||
| "details": {"id": sub_id, "job_status_id": job_status_id}, | ||||||
| "status": "accepted", | ||||||
| }, | ||||||
| ) | ||||||
| # Preserve FastAPI HTTPException as-is | ||||||
| except HTTPException: | ||||||
|
|
@@ -470,6 +499,7 @@ async def run_submission_async( | |||||
| logger.error(f"Unexpected error in api submissoin: {e}") | ||||||
|
||||||
| logger.error(f"Unexpected error in api submissoin: {e}") | |
| logger.error(f"Unexpected error in api submission: {e}") |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -122,6 +122,16 @@ def __init__(self, bot: "ClusterBot"): | |||||||||
| name="set-forum-ids", description="Sets forum IDs" | ||||||||||
| )(self.set_forum_ids) | ||||||||||
|
|
||||||||||
| self.set_submission_rate_limit = bot.admin_group.command( | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my thinking is we would instead say something like "you can't submit anything new, until your last request goes through or some timeout expires" - please reach out to mods if you'd like to be overriden |
||||||||||
| name="set-submission-rate-limit", | ||||||||||
| description="Set default or per-user submission rate limit (submissions/minute).", | ||||||||||
| )(self.set_submission_rate_limit) | ||||||||||
|
|
||||||||||
| self.get_submission_rate_limit = bot.admin_group.command( | ||||||||||
| name="get-submission-rate-limit", | ||||||||||
| description="Get default or per-user submission rate limit (submissions/minute).", | ||||||||||
| )(self.get_submission_rate_limit) | ||||||||||
|
|
||||||||||
| self._scheduled_cleanup_temp_users.start() | ||||||||||
|
|
||||||||||
| # -------------------------------------------------------------------------- | ||||||||||
|
|
@@ -512,6 +522,153 @@ async def start(self, interaction: discord.Interaction): | |||||||||
| interaction, "Bot will accept submissions again!", ephemeral=True | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| def _parse_user_id_arg(self, user_id: str) -> str: | ||||||||||
| """Accepts a raw id or a discord mention and returns the id string.""" | ||||||||||
| s = (user_id or "").strip() | ||||||||||
| if s.startswith("<@") and s.endswith(">"): | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you have some example tests for this function? |
||||||||||
| s = s[2:-1].strip() | ||||||||||
| if s.startswith("!"): | ||||||||||
| s = s[1:].strip() | ||||||||||
| return s | ||||||||||
|
|
||||||||||
| def _format_rate(self, rate: float | None) -> str: | ||||||||||
| if rate is None: | ||||||||||
| return "unlimited" | ||||||||||
| r = float(rate) | ||||||||||
| if r == 0: | ||||||||||
| return "blocked" | ||||||||||
| return f"{r:g}/min" | ||||||||||
|
|
||||||||||
| @app_commands.describe( | ||||||||||
| rate_per_minute="Rate in submissions/minute. Use 'none' for unlimited; 'default' clears a user override.", # noqa: E501 | ||||||||||
| user_id="Optional user id or mention. If omitted, sets the default.", | ||||||||||
| ) | ||||||||||
| @with_error_handling | ||||||||||
| async def set_submission_rate_limit( | ||||||||||
| self, | ||||||||||
| interaction: discord.Interaction, | ||||||||||
| rate_per_minute: str, | ||||||||||
| user_id: Optional[str] = None, | ||||||||||
| ): | ||||||||||
| is_admin = await self.admin_check(interaction) | ||||||||||
| if not is_admin: | ||||||||||
| await send_discord_message( | ||||||||||
| interaction, | ||||||||||
| "You need to be Admin to use this command.", | ||||||||||
| ephemeral=True, | ||||||||||
| ) | ||||||||||
| return | ||||||||||
|
|
||||||||||
| rate_s = (rate_per_minute or "").strip().lower() | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code is too defensive, it's mainly us mods making the change right? in which case we don't need to be this paranaoid Also now as a mod i'm not sure what value should i set closer to 1 or 0.5, it's partly why I favor unlimited or at most one submission just cause it's easier for me to intuit what to put in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code is too defensive, it's mainly us mods making the change right? in which case we don't need to be this paranaoid Also now as a mod i'm not sure what value should i set closer to 1 or 0.5, it's partly why I favor unlimited or at most one submission just cause it's easier for me to intuit what to put in |
||||||||||
| if rate_s in {"none", "unlimited", "off"}: | ||||||||||
| parsed_rate: float | None = None | ||||||||||
| clear_override = False | ||||||||||
| elif rate_s == "default": | ||||||||||
| parsed_rate = None | ||||||||||
| clear_override = True | ||||||||||
| else: | ||||||||||
| try: | ||||||||||
| parsed_rate = float(rate_s) | ||||||||||
| except ValueError: | ||||||||||
| await send_discord_message( | ||||||||||
| interaction, | ||||||||||
| "Invalid rate. Use a number like `1` or `0.5`, or `none`.", | ||||||||||
| ephemeral=True, | ||||||||||
| ) | ||||||||||
| return | ||||||||||
| if parsed_rate < 0: | ||||||||||
| await send_discord_message( | ||||||||||
| interaction, | ||||||||||
| "Invalid rate. Must be >= 0 (or `none`).", | ||||||||||
| ephemeral=True, | ||||||||||
| ) | ||||||||||
| return | ||||||||||
| clear_override = False | ||||||||||
|
|
||||||||||
| with self.bot.leaderboard_db as db: | ||||||||||
| if user_id is None or user_id.strip() == "": | ||||||||||
| if clear_override: | ||||||||||
| await send_discord_message( | ||||||||||
| interaction, | ||||||||||
| "For default limit, use a number or `none` (not `default`).", | ||||||||||
|
||||||||||
| "For default limit, use a number or `none` (not `default`).", | |
| "For the default submission limit, use a number or `none` (unlimited). " | |
| "The special value `default` is only valid when setting a per-user limit, " | |
| "where it clears that user's override so the default applies.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make sure,
it seems there is no creation for user limit if users only has web access
db.set_user_submission_rate_limit seems only happens for discrod.
I think for web/cli, you also need
- check if it's admin
- if it's not and the user's limit not in db, create it
- if exist, check remaining
meanwhile, i will update rate limit on FE side to be 10/minute too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1