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: 1182 prevent used resources from deletion #1244

Merged
merged 12 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions agenta-backend/agenta_backend/models/api/evaluation_model.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from enum import Enum
from datetime import datetime
from pydantic import BaseModel, Field
from pydantic import BaseModel
from typing import Optional, List, Dict, Any, Union
from agenta_backend.models.api.api_models import Result

Expand Down Expand Up @@ -39,7 +39,7 @@ class EvaluationScenarioStatusEnum(str, Enum):


class AggregatedResult(BaseModel):
evaluator_config: EvaluatorConfig
evaluator_config: Union[EvaluatorConfig, Dict[str, Any]]
result: Result


Expand All @@ -66,8 +66,8 @@ class Evaluation(BaseModel):
variant_names: List[str]
variant_revision_ids: List[str]
revisions: List[str]
testset_id: str
testset_name: str
testset_id: Optional[str]
testset_name: Optional[str]
status: Result
aggregated_results: List[AggregatedResult]
created_at: datetime
Expand Down
32 changes: 25 additions & 7 deletions agenta-backend/agenta_backend/models/converters.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
)

import logging
from beanie import Link

logger = logging.getLogger(__name__)
logger.setLevel(logging.DEBUG)
Expand Down Expand Up @@ -87,6 +88,9 @@ async def evaluation_db_to_pydantic(
str(evaluation_db.variant_revision)
)
revision = str(variant_revision.revision)
aggregated_results = await aggregated_result_to_pydantic(
evaluation_db.aggregated_results
)
return Evaluation(
id=str(evaluation_db.id),
app_id=str(evaluation_db.app.id),
Expand All @@ -97,11 +101,15 @@ async def evaluation_db_to_pydantic(
variant_revision_ids=[str(evaluation_db.variant_revision)],
revisions=[revision],
variant_names=[variant_name],
testset_id=str(evaluation_db.testset.id),
testset_name=evaluation_db.testset.name,
aggregated_results=await aggregated_result_to_pydantic(
evaluation_db.aggregated_results
testset_id=(
"" if type(evaluation_db.testset) is Link else str(evaluation_db.testset.id)
),
testset_name=(
""
if type(evaluation_db.testset) is Link
else str(evaluation_db.testset.name)
),
aggregated_results=aggregated_results,
created_at=evaluation_db.created_at,
updated_at=evaluation_db.updated_at,
)
Expand Down Expand Up @@ -132,13 +140,19 @@ async def human_evaluation_db_to_pydantic(
evaluation_type=evaluation_db.evaluation_type,
variant_ids=[str(variant) for variant in evaluation_db.variants],
variant_names=variant_names,
testset_id=(
"" if type(evaluation_db.testset) is Link else str(evaluation_db.testset.id)
),
testset_name=(
""
if type(evaluation_db.testset) is Link
else str(evaluation_db.testset.name)
),
variants_revision_ids=[
str(variant_revision)
for variant_revision in evaluation_db.variants_revisions
],
revisions=revisions,
testset_id=str(evaluation_db.testset.id),
testset_name=evaluation_db.testset.name,
created_at=evaluation_db.created_at,
updated_at=evaluation_db.updated_at,
)
Expand Down Expand Up @@ -171,7 +185,11 @@ async def aggregated_result_to_pydantic(results: List[AggregatedResult]) -> List
)
transformed_results.append(
{
"evaluator_config": json.loads(evaluator_config_dict),
"evaluator_config": (
{}
if evaluator_config_dict is None
else json.loads(evaluator_config_dict)
),
"result": result.result.dict(),
}
)
Expand Down
44 changes: 42 additions & 2 deletions agenta-backend/agenta_backend/routers/evaluation_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
from typing import Any, List

from fastapi.responses import JSONResponse
from fastapi.encoders import jsonable_encoder
from fastapi import HTTPException, Request, status, Response
from fastapi import HTTPException, Request, status, Response, Query
from beanie import PydanticObjectId as ObjectId

from agenta_backend.utils.common import APIRouter
from agenta_backend.models.api.evaluation_model import (
Expand Down Expand Up @@ -36,6 +36,46 @@
router = APIRouter()


@router.get(
"/by_resource/",
response_model=List[ObjectId],
)
async def fetch_evaluation_ids(
app_id: str,
resource_type: str,
request: Request,
resource_ids: List[str] = Query(None),
):
"""Fetches evaluation ids for a given resource type and id.

Arguments:
app_id (str): The ID of the app for which to fetch evaluations.
resource_type (str): The type of resource for which to fetch evaluations.
resource_ids List[ObjectId]: The IDs of resource for which to fetch evaluations.

Raises:
HTTPException: If the resource_type is invalid or access is denied.

Returns:
List[str]: A list of evaluation ids.
"""
user_org_data: dict = await get_user_and_org_id(request.state.user_id)
access_app = await check_access_to_app(
user_org_data=user_org_data,
app_id=app_id,
)
if not access_app:
raise HTTPException(
status_code=403,
detail=f"You do not have access to this app: {str(app_id)}",
)

evaluations = await evaluation_service.fetch_evaluations_by_resource(
resource_type, resource_ids
)
return list(map(lambda x: x.id, evaluations))


@router.post("/", response_model=List[Evaluation], operation_id="create_evaluation")
async def create_evaluation(
payload: NewEvaluation,
Expand Down
22 changes: 22 additions & 0 deletions agenta-backend/agenta_backend/services/evaluation_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
)

from beanie import PydanticObjectId as ObjectId
from beanie.operators import In


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -817,3 +818,24 @@ def remove_duplicates(csvdata):
unique_entries.append(entry)

return unique_entries


async def fetch_evaluations_by_resource(resource_type: str, resource_ids: List[str]):
ids = list(map(lambda x: ObjectId(x), resource_ids))
if resource_type == "variant":
res = await EvaluationDB.find(In(EvaluationDB.variant, ids)).to_list()
elif resource_type == "testset":
res = await EvaluationDB.find(In(EvaluationDB.testset.id, ids)).to_list()
elif resource_type == "evaluator_config":
res = await EvaluationDB.find(
In(
EvaluationDB.evaluators_configs,
ids,
)
).to_list()
else:
raise HTTPException(
status_code=400,
detail=f"resource_type {resource_type} is not supported",
)
return res
12 changes: 12 additions & 0 deletions agenta-web/src/components/Playground/Playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {arrayMove, SortableContext, horizontalListSortingStrategy} from "@dnd-ki
import DraggableTabNode from "../DraggableTabNode/DraggableTabNode"
import {useLocalStorage} from "usehooks-ts"
import TestContextProvider from "./TestContextProvider"
import {checkIfResourceValidForDeletion} from "@/lib/helpers/evaluate"
import ResultComponent from "../ResultComponent/ResultComponent"

const Playground: React.FC = () => {
Expand Down Expand Up @@ -206,6 +207,17 @@ const Playground: React.FC = () => {
},
onOk: async () => {
try {
const variantId =
variants.find((item) => item.variantId === tabID.current)?.variantId ||
variants.find((item) => item.variantName === activeKey)?.variantId
if (
variantId &&
!(await checkIfResourceValidForDeletion({
resourceType: "variant",
resourceIds: [variantId],
}))
)
return
if (deleteAction) await deleteAction()
removeTab()
messageApi.open({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,14 @@ const EvaluationScenarios: React.FC<Props> = () => {
})
scenarios[0]?.evaluators_configs.forEach((config, index) => {
colDefs.push({
headerName: config.name,
headerName: config?.name,
headerComponent: (props: any) => {
const evaluator = evaluators.find((item) => item.key === config.evaluator_key)!
const evaluator = evaluators.find((item) => item.key === config?.evaluator_key)!
return (
<AgCustomHeader {...props}>
<Space direction="vertical" style={{padding: "0.5rem 0"}}>
<span>{config.name}</span>
<Tag color={evaluator.color}>{evaluator.name}</Tag>
<Tag color={evaluator?.color}>{evaluator?.name}</Tag>
</Space>
</AgCustomHeader>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import AlertPopup from "@/components/AlertPopup/AlertPopup"
import {deleteEvaluatorConfig} from "@/services/evaluations"
import {useAtom} from "jotai"
import {evaluatorsAtom} from "@/lib/atoms/evaluation"
import {checkIfResourceValidForDeletion} from "@/lib/helpers/evaluate"

const useStyles = createUseStyles((theme: JSSTheme) => ({
card: {
Expand Down Expand Up @@ -60,14 +61,23 @@ const EvaluatorCard: React.FC<Props> = ({evaluatorConfig, onEdit, onSuccessDelet
const [evaluators] = useAtom(evaluatorsAtom)
const evaluator = evaluators.find((item) => item.key === evaluatorConfig.evaluator_key)!

const onDelete = () => {
const onDelete = async () => {
AlertPopup({
title: "Delete evaluator",
message: "Are you sure you want to delete this evaluator?",
onOk: () =>
deleteEvaluatorConfig(evaluatorConfig.id)
.then(onSuccessDelete)
.catch(console.error),
onOk: async () => {
if (
!(await checkIfResourceValidForDeletion({
resourceType: "evaluator_config",
resourceIds: [evaluatorConfig.id],
}))
)
return
try {
await deleteEvaluatorConfig(evaluatorConfig.id)
onSuccessDelete?.()
} catch (error) {}
},
})
}

Expand Down
30 changes: 30 additions & 0 deletions agenta-web/src/lib/helpers/evaluate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import {
EvaluationScenario,
} from "../Types"
import {convertToCsv, downloadCsv} from "./fileManipulations"
import {fetchEvaluatonIdsByResource} from "@/services/evaluations"
import {getAppValues} from "@/contexts/app.context"
import AlertPopup from "@/components/AlertPopup/AlertPopup"
import {capitalize, round} from "lodash"
import dayjs from "dayjs"
import {runningStatuses} from "@/components/pages/evaluations/cellRenderers/cellRenderers"
Expand Down Expand Up @@ -231,6 +234,33 @@ export const getVotesPercentage = (record: HumanEvaluationListTableDataType, ind
return record.votesData.variants_votes_data[variant]?.percentage
}

export const checkIfResourceValidForDeletion = async (
data: Omit<Parameters<typeof fetchEvaluatonIdsByResource>[0], "appId">,
) => {
const appId = getAppValues().currentApp?.app_id
if (!appId) return false

const response = await fetchEvaluatonIdsByResource({...data, appId})
if (response.data.length > 0) {
const name =
(data.resourceType === "testset"
? "Testset"
: data.resourceType === "evaluator_config"
? "Evaluator"
: "Variant") + (data.resourceIds.length > 1 ? "s" : "")

const suffix = response.data.length > 1 ? "s" : ""
AlertPopup({
title: `${name} is in use`,
message: `The ${name} is currently in used by ${response.data.length} evaluation${suffix}. Please delete the evaluation${suffix} first.`,
cancelText: null,
okText: "Ok",
})
return false
}
return true
}

export function getTypedValue(res?: TypedValue) {
const {value, type, error} = res || {}
if (type === "error") {
Expand Down
11 changes: 10 additions & 1 deletion agenta-web/src/lib/services/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,16 @@ export async function updateTestset(testsetId: String, testsetName: string, test
return response
}

export const loadTestset = async (testsetId: string) => {
export const loadTestset = async (testsetId: string | null) => {
if (!testsetId) {
return {
id: undefined,
name: "No Test Set Associated",
created_at: "",
updated_at: "",
csvdata: [],
}
}
const response = await axios.get(`${getAgentaApiUrl()}/api/testsets/${testsetId}/`)
return response.data
}
Expand Down
8 changes: 8 additions & 0 deletions agenta-web/src/pages/apps/[app_id]/testsets/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {deleteTestsets, useLoadTestsetsList} from "@/lib/services/api"
import {createUseStyles} from "react-jss"
import {testset} from "@/lib/Types"
import {isDemo} from "@/lib/helpers/utils"
import {checkIfResourceValidForDeletion} from "@/lib/helpers/evaluate"

const useStyles = createUseStyles({
container: {
Expand Down Expand Up @@ -71,6 +72,13 @@ export default function Testsets() {
const onDelete = async () => {
const testsetsIds = selectedRowKeys.map((key) => key.toString())
try {
if (
!(await checkIfResourceValidForDeletion({
resourceType: "testset",
resourceIds: testsetsIds,
}))
)
return
await deleteTestsets(testsetsIds)
mutate()
setSelectedRowKeys([])
Expand Down
20 changes: 19 additions & 1 deletion agenta-web/src/services/evaluations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ export const updateAnnotationScenario = async (
// Comparison
export const fetchAllComparisonResults = async (evaluationIds: string[]) => {
const scenarioGroups = await Promise.all(evaluationIds.map(fetchAllEvaluationScenarios))
const testset: TestSet = await loadTestset(scenarioGroups[0][0].evaluation.testset.id)
const testset: TestSet = await loadTestset(scenarioGroups[0][0].evaluation?.testset?.id)

const inputsNameSet = new Set<string>()
scenarioGroups.forEach((group) => {
Expand Down Expand Up @@ -267,3 +267,21 @@ export const fetchAllComparisonResults = async (evaluationIds: string[]) => {
evaluations: scenarioGroups.map((group) => group[0].evaluation),
}
}

// Evaluation IDs by resource
export const fetchEvaluatonIdsByResource = async ({
resourceIds,
resourceType,
appId,
}: {
resourceIds: string[]
resourceType: "testset" | "evaluator_config" | "variant"
appId: string
}) => {
return axios.get(`/api/evaluations/by_resource`, {
params: {resource_ids: resourceIds, resource_type: resourceType, app_id: appId},
paramsSerializer: {
indexes: null, //no brackets in query params
},
})
}
Loading