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

Set a default for zarr_dir #1990

Merged
merged 27 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
eeab6e8
test reusing id
ychiucco Oct 30, 2024
acbe909
user settings project dir
ychiucco Oct 30, 2024
8bbbc08
dataset zarr dir optional
ychiucco Oct 30, 2024
7da666f
skip test if sqlite
ychiucco Oct 30, 2024
6ca8db7
remove some optionals and slugify names
ychiucco Oct 30, 2024
c1a55ff
change endpoint if-else branching
ychiucco Oct 30, 2024
fa22216
use string tool
ychiucco Oct 30, 2024
b8d7ca5
fix test
ychiucco Oct 30, 2024
5861ebb
exclude none and test
ychiucco Oct 30, 2024
b1cb677
fix tests
ychiucco Oct 31, 2024
a77af50
accept the edge-case of mistakenly reusing folders in sqlite
ychiucco Nov 4, 2024
db3cec4
correctly remove sqlite check
ychiucco Nov 4, 2024
c852975
Merge branch 'main' into 1934-set-a-default-for-zarr-dir
ychiucco Nov 4, 2024
a48fee1
changelog
ychiucco Nov 4, 2024
d81c75a
DatasetImportV2.zarr_dir made required
ychiucco Nov 4, 2024
810ca16
remove exclude_none and setattr
ychiucco Nov 4, 2024
d612e4b
docstrin for superuser restricted schemas
ychiucco Nov 4, 2024
4d9eb33
normalize path
ychiucco Nov 4, 2024
d72a049
replace skip with if-else
ychiucco Nov 4, 2024
12580d9
fix default factory
ychiucco Nov 4, 2024
9845208
assert zarr_dir
ychiucco Nov 4, 2024
fc5e5ad
TO REVERT
ychiucco Nov 4, 2024
2104143
Revert "TO REVERT"
ychiucco Nov 4, 2024
ee59118
tests that `None` is a valid `project_dir`
ychiucco Nov 4, 2024
5d797dd
test missing case
ychiucco Nov 4, 2024
dfc77ff
Merge branch 'main' into 1934-set-a-default-for-zarr-dir
ychiucco Nov 4, 2024
a173d8d
Merge branch 'main' into 1934-set-a-default-for-zarr-dir
ychiucco Nov 4, 2024
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
* Now `pip install` uses `--no-cache` (\#1980).
* API
* Deprecate the `verbose` query parameter in `GET /api/v2/task/collect/{state_id}/` (\#1980).
* Add `project_dir` attribute to `UserSettings` (\#1990).
* Set a default for `DatasetV2.zarr_dir` (\#1990).
* Combine the `args_schema_parallel` and `args_schema_non_parallel` query parameters in `GET /api/v2/task/` into a single parameter `args_schema` (\#1998).

# 2.7.1
Expand Down
1 change: 1 addition & 0 deletions fractal_server/app/models/user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,4 @@ class UserSettings(SQLModel, table=True):
ssh_jobs_dir: Optional[str] = None
slurm_user: Optional[str] = None
cache_dir: Optional[str] = None
project_dir: Optional[str] = None
45 changes: 39 additions & 6 deletions fractal_server/app/routes/api/v2/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from ._aux_functions import _get_submitted_jobs_statement
from fractal_server.app.models import UserOAuth
from fractal_server.app.routes.auth import current_active_user
from fractal_server.string_tools import sanitize_string
from fractal_server.urls import normalize_url

router = APIRouter()

Expand All @@ -40,14 +42,45 @@ async def create_dataset(
"""
Add new dataset to current project
"""
await _get_project_check_owner(
project = await _get_project_check_owner(
project_id=project_id, user_id=user.id, db=db
)
db_dataset = DatasetV2(project_id=project_id, **dataset.dict())
db.add(db_dataset)
await db.commit()
await db.refresh(db_dataset)
await db.close()

if dataset.zarr_dir is None:

if user.settings.project_dir is None:
raise HTTPException(
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
detail=(
"Both 'dataset.zarr_dir' and 'user.settings.project_dir' "
"are null"
),
)

db_dataset = DatasetV2(
project_id=project_id,
zarr_dir="__PLACEHOLDER__",
**dataset.dict(exclude={"zarr_dir"}),
)
db.add(db_dataset)
await db.commit()
await db.refresh(db_dataset)
path = (
f"{user.settings.project_dir}/fractal/"
f"{project_id}_{sanitize_string(project.name)}/"
f"{db_dataset.id}_{sanitize_string(db_dataset.name)}"
)
normalized_path = normalize_url(path)
db_dataset.zarr_dir = normalized_path

db.add(db_dataset)
await db.commit()
await db.refresh(db_dataset)
else:
db_dataset = DatasetV2(project_id=project_id, **dataset.dict())
db.add(db_dataset)
await db.commit()
await db.refresh(db_dataset)

return db_dataset

Expand Down
18 changes: 18 additions & 0 deletions fractal_server/app/schemas/user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@


class UserSettingsRead(BaseModel):
"""
Schema reserved for superusers
"""

id: int
ssh_host: Optional[str] = None
ssh_username: Optional[str] = None
Expand All @@ -28,16 +32,22 @@ class UserSettingsRead(BaseModel):
slurm_user: Optional[str] = None
slurm_accounts: list[str]
cache_dir: Optional[str] = None
project_dir: Optional[str] = None


class UserSettingsReadStrict(BaseModel):
slurm_user: Optional[str] = None
slurm_accounts: list[str]
cache_dir: Optional[str] = None
ssh_username: Optional[str] = None
project_dir: Optional[str] = None


class UserSettingsUpdate(BaseModel, extra=Extra.forbid):
"""
Schema reserved for superusers
"""

ssh_host: Optional[str] = None
ssh_username: Optional[str] = None
ssh_private_key_path: Optional[str] = None
Expand All @@ -46,6 +56,7 @@ class UserSettingsUpdate(BaseModel, extra=Extra.forbid):
slurm_user: Optional[str] = None
slurm_accounts: Optional[list[StrictStr]] = None
cache_dir: Optional[str] = None
project_dir: Optional[str] = None

_ssh_host = validator("ssh_host", allow_reuse=True)(
valstr("ssh_host", accept_none=True)
Expand Down Expand Up @@ -83,6 +94,13 @@ def cache_dir_validator(cls, value):
validate_cmd(value)
return val_absolute_path("cache_dir")(value)

@validator("project_dir")
def project_dir_validator(cls, value):
if value is None:
return None
validate_cmd(value)
return val_absolute_path("project_dir")(value)


class UserSettingsUpdateStrict(BaseModel, extra=Extra.forbid):
slurm_accounts: Optional[list[StrictStr]] = None
Expand Down
8 changes: 5 additions & 3 deletions fractal_server/app/schemas/v2/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,16 @@ class DatasetCreateV2(BaseModel, extra=Extra.forbid):

name: str

zarr_dir: str
zarr_dir: Optional[str] = None

filters: Filters = Field(default_factory=Filters)

# Validators
@validator("zarr_dir")
def normalize_zarr_dir(cls, v: str) -> str:
return normalize_url(v)
if v is not None:
return normalize_url(v)
return v

_name = validator("name", allow_reuse=True)(valstr("name"))

Expand Down Expand Up @@ -95,7 +97,7 @@ class DatasetImportV2(BaseModel, extra=Extra.forbid):

name: str
zarr_dir: str
images: list[SingleImage] = Field(default_factory=[])
images: list[SingleImage] = Field(default_factory=list)
filters: Filters = Field(default_factory=Filters)

# Validators
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
"""user settings project dir

Revision ID: 19eca0dd47a9
Revises: 8e8f227a3e36
Create Date: 2024-10-30 14:34:28.219355

"""
import sqlalchemy as sa
import sqlmodel
from alembic import op


# revision identifiers, used by Alembic.
revision = "19eca0dd47a9"
down_revision = "8e8f227a3e36"
branch_labels = None
depends_on = None


def upgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
with op.batch_alter_table("user_settings", schema=None) as batch_op:
batch_op.add_column(
sa.Column(
"project_dir",
sqlmodel.sql.sqltypes.AutoString(),
nullable=True,
)
)

# ### end Alembic commands ###


def downgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
with op.batch_alter_table("user_settings", schema=None) as batch_op:
batch_op.drop_column("project_dir")

# ### end Alembic commands ###
26 changes: 26 additions & 0 deletions tests/no_version/test_api_user_groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ async def test_patch_user_settings_bulk(
slurm_user="test01",
slurm_accounts=[],
cache_dir=None,
project_dir=None,
) == user.settings.dict(exclude={"id"})

# remove user4 from default user group
Expand All @@ -351,6 +352,7 @@ async def test_patch_user_settings_bulk(
# missing `slurm_user`
slurm_accounts=["foo", "bar"],
cache_dir="/tmp/cache",
project_dir="/foo",
)
res = await registered_superuser_client.patch(
f"{PREFIX}/group/{default_user_group.id}/user-settings/", json=patch
Expand All @@ -373,4 +375,28 @@ async def test_patch_user_settings_bulk(
slurm_user="test01",
slurm_accounts=[],
cache_dir=None,
project_dir=None,
) == user4.settings.dict(exclude={"id"})

res = await registered_superuser_client.patch(
f"{PREFIX}/group/{default_user_group.id}/user-settings/",
json=dict(project_dir="not/an/absolute/path"),
)
assert res.status_code == 422

# `None` is a valid `project_dir`
res = await registered_superuser_client.patch(
f"{PREFIX}/group/{default_user_group.id}/user-settings/",
json=dict(project_dir="/fancy/dir"),
)
assert res.status_code == 200
for user in [user1, user2, user3]:
await db.refresh(user)
assert user.settings.project_dir == "/fancy/dir"
res = await registered_superuser_client.patch(
f"{PREFIX}/group/{default_user_group.id}/user-settings/",
json=dict(project_dir=None),
)
for user in [user1, user2, user3]:
await db.refresh(user)
assert user.settings.project_dir is None
53 changes: 53 additions & 0 deletions tests/no_version/test_db.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest
from devtools import debug
from sqlmodel import delete
from sqlmodel import select

from fractal_server.app.db import DB
Expand Down Expand Up @@ -114,3 +115,55 @@ def test_DB_class_sync():

assert DB._engine_sync
assert DB._sync_session_maker


async def test_reusing_id(db):
"""
Tests different database behaviors with incremental IDs.

https://github.com/fractal-analytics-platform/fractal-server/issues/1991
"""

num_users = 10

# Create `num_users` new users
user_list = [
UserOAuth(email=f"{x}@y.z", hashed_password="xxx")
for x in range(num_users)
]

for user in user_list:
db.add(user)
await db.commit()
for user in user_list:
await db.refresh(user)

# Extract list of IDs
user_id_list = [user.id for user in user_list]

# Remove all users
await db.execute(delete(UserOAuth))
res = await db.execute(select(UserOAuth))
users = res.scalars().unique().all()
assert len(users) == 0

# Create `num_users + 10` new users
new_user_list = [
UserOAuth(email=f"{x}@y.z", hashed_password="xxx")
for x in range(num_users + 10)
]
for user in new_user_list:
db.add(user)
await db.commit()
for user in new_user_list:
await db.refresh(user)

# Extract list of IDs
new_user_id_list = [user.id for user in new_user_list]

if DB_ENGINE == "sqlite":
# Assert IDs lists are overlapping
assert not set(new_user_id_list).isdisjoint(set(user_id_list))
else:
# Assert IDs lists are disjoined
assert set(new_user_id_list).isdisjoint(set(user_id_list))
5 changes: 5 additions & 0 deletions tests/v2/01_schemas/test_schemas_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ async def test_schemas_dataset_v2():
DatasetCreateV2(
name="name", zarr_dir="/zarr", filters={"types": {"a": "b"}}
)
# Test zarr_dir=None is valid
DatasetCreateV2(name="name", zarr_dir=None)

dataset_create = DatasetCreateV2(
name="name",
Expand All @@ -35,6 +37,9 @@ async def test_schemas_dataset_v2():
)
assert dataset_create.zarr_dir == normalize_url(dataset_create.zarr_dir)

with pytest.raises(ValidationError):
DatasetImportV2(name="name", zarr_dir=None)

dataset_import = DatasetImportV2(
name="name",
filters={"attributes": {"x": 10}},
Expand Down
Loading
Loading