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

Remove UserSettings.cache_dir #2121

Merged
merged 7 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

# Unreleased

* API
ychiucco marked this conversation as resolved.
Show resolved Hide resolved
* Remove `cache_dir` and use `project_dir/.fractal_cache` (\#2121).
* Testing:
* Re-include a specific test previously skipped for Python 3.12 (\#2114).
* Add metadata to `fractal-tasks-mock` package (\#2117).
Expand Down
1 change: 0 additions & 1 deletion fractal_server/app/models/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ class UserOAuth(SQLModel, table=True):
is_verified:
slurm_user:
slurm_accounts:
cache_dir:
username:
oauth_accounts:
"""
Expand Down
2 changes: 0 additions & 2 deletions fractal_server/app/models/user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class UserSettings(SQLModel, table=True):
ssh_tasks_dir: Task-venvs base folder on `ssh_host`.
ssh_jobs_dir: Jobs base folder on `ssh_host`.
slurm_user: Local user, to be impersonated via `sudo -u`
cache_dir: Folder where `slurm_user` can write.
ychiucco marked this conversation as resolved.
Show resolved Hide resolved
"""

__tablename__ = "user_settings"
Expand All @@ -35,5 +34,4 @@ class UserSettings(SQLModel, table=True):
ssh_tasks_dir: Optional[str] = None
ssh_jobs_dir: Optional[str] = None
slurm_user: Optional[str] = None
cache_dir: Optional[str] = None
project_dir: Optional[str] = None
8 changes: 7 additions & 1 deletion fractal_server/app/routes/api/v1/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,12 @@ async def apply_workflow(
await db.commit()
await db.refresh(job)

cache_dir = (
f"{user_settings.project_dir}/.fractal_cache"
if user_settings.project_dir is not None
else None
)

background_tasks.add_task(
submit_workflow,
workflow_id=workflow.id,
Expand All @@ -456,7 +462,7 @@ async def apply_workflow(
job_id=job.id,
worker_init=apply_workflow.worker_init,
slurm_user=user_settings.slurm_user,
user_cache_dir=user_settings.cache_dir,
user_cache_dir=cache_dir,
)
request.app.state.jobsV1.append(job.id)
logger.info(
Expand Down
14 changes: 9 additions & 5 deletions fractal_server/app/routes/api/v2/submit.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,18 +186,22 @@ async def apply_workflow(
f"_{timestamp_string}"
)

cache_dir = (
f"{user_settings.project_dir}/.fractal_cache"
ychiucco marked this conversation as resolved.
Show resolved Hide resolved
if user_settings.project_dir is not None
else None
)

# Define user-side job directory
if FRACTAL_RUNNER_BACKEND == "local":
WORKFLOW_DIR_REMOTE = WORKFLOW_DIR_LOCAL
elif FRACTAL_RUNNER_BACKEND == "local_experimental":
WORKFLOW_DIR_REMOTE = WORKFLOW_DIR_LOCAL
elif FRACTAL_RUNNER_BACKEND == "slurm":
WORKFLOW_DIR_REMOTE = (
Path(user_settings.cache_dir) / f"{WORKFLOW_DIR_LOCAL.name}"
)
WORKFLOW_DIR_REMOTE = Path(cache_dir) / WORKFLOW_DIR_LOCAL.name
elif FRACTAL_RUNNER_BACKEND == "slurm_ssh":
WORKFLOW_DIR_REMOTE = (
Path(user_settings.ssh_jobs_dir) / f"{WORKFLOW_DIR_LOCAL.name}"
Path(user_settings.ssh_jobs_dir) / WORKFLOW_DIR_LOCAL.name
)

# Update job folders in the db
Expand Down Expand Up @@ -229,7 +233,7 @@ async def apply_workflow(
user_settings=user_settings,
worker_init=job.worker_init,
slurm_user=user_settings.slurm_user,
user_cache_dir=user_settings.cache_dir,
user_cache_dir=cache_dir,
fractal_ssh=fractal_ssh,
)
request.app.state.jobsV2.append(job.id)
Expand Down
18 changes: 0 additions & 18 deletions fractal_server/app/schemas/user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,12 @@ class UserSettingsRead(BaseModel):
ssh_jobs_dir: Optional[str] = None
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

Expand All @@ -55,7 +53,6 @@ class UserSettingsUpdate(BaseModel, extra=Extra.forbid):
ssh_jobs_dir: Optional[str] = None
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)(
Expand Down Expand Up @@ -87,13 +84,6 @@ def slurm_accounts_validator(cls, value):
value[i] = valstr(f"slurm_accounts[{i}]")(item)
return val_unique_list("slurm_accounts")(value)

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

@validator("project_dir")
def project_dir_validator(cls, value):
if value is None:
Expand All @@ -104,15 +94,7 @@ def project_dir_validator(cls, value):

class UserSettingsUpdateStrict(BaseModel, extra=Extra.forbid):
slurm_accounts: Optional[list[StrictStr]] = None
cache_dir: Optional[str] = None

_slurm_accounts = validator("slurm_accounts", allow_reuse=True)(
val_unique_list("slurm_accounts")
)

@validator("cache_dir")
def cache_dir_validator(cls, value):
if value is None:
return value
validate_cmd(value)
return val_absolute_path("cache_dir")(value)
4 changes: 2 additions & 2 deletions fractal_server/app/user_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ class SlurmSudoUserSettings(BaseModel):

Attributes:
slurm_user: User to be impersonated via `sudo -u`.
cache_dir: Folder where `slurm_user` can write.
project_dir: Folder where `slurm_user` can write.
slurm_accounts:
List of SLURM accounts, to be used upon Fractal job submission.
"""

slurm_user: str
cache_dir: str
project_dir: str
slurm_accounts: list[str]
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
"""Remove UserSettings.cache_dir

Revision ID: 316140ff7ee1
Revises: d256a7379ab8
Create Date: 2024-12-03 10:15:53.255958

"""
import sqlalchemy as sa
from alembic import op


# revision identifiers, used by Alembic.
revision = "316140ff7ee1"
down_revision = "d256a7379ab8"
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.drop_column("cache_dir")

# ### 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.add_column(
sa.Column(
"cache_dir", sa.VARCHAR(), autoincrement=False, nullable=True
)
)

# ### end Alembic commands ###
6 changes: 2 additions & 4 deletions tests/no_version/test_api_current_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,7 @@ async def test_patch_current_user_no_extra(registered_client):
"""
res = await registered_client.patch(PREFIX, json={})
assert res.status_code == 200
res = await registered_client.patch(
PREFIX, json={"cache_dir": "/tmp", "foo": "bar"}
)
res = await registered_client.patch(PREFIX, json={"foo": "bar"})
assert res.status_code == 422


Expand Down Expand Up @@ -178,7 +176,7 @@ async def test_get_and_patch_current_user_settings(registered_client):
else:
assert v is None

patch = dict(slurm_accounts=["foo", "bar"], cache_dir="/tmp/foo_cache")
patch = dict(slurm_accounts=["foo", "bar"])
res = await registered_client.patch(f"{PREFIX}settings/", json=patch)
assert res.status_code == 200

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

Expand All @@ -353,7 +352,6 @@ async def test_patch_user_settings_bulk(
ssh_jobs_dir="/tmp/job",
# missing `slurm_user`
slurm_accounts=["foo", "bar"],
cache_dir="/tmp/cache",
project_dir="/foo",
)
res = await registered_superuser_client.patch(
Expand All @@ -376,7 +374,6 @@ async def test_patch_user_settings_bulk(
ssh_jobs_dir=None,
slurm_user="test01",
slurm_accounts=[],
cache_dir=None,
project_dir=None,
) == user4.settings.dict(exclude={"id"})

Expand Down
1 change: 0 additions & 1 deletion tests/no_version/test_api_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,6 @@ async def test_get_and_patch_user_settings(registered_superuser_client):
# missing "ssh_jobs_dir"
slurm_user="fractal",
slurm_accounts=["foo", "bar"],
cache_dir="/tmp/cache",
)
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{user_id}/settings/", json=patch
Expand Down
5 changes: 1 addition & 4 deletions tests/no_version/test_fixture_MockCurrentUser.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,19 @@
from devtools import debug


@pytest.mark.parametrize("cache_dir", ("/some/path", None))
@pytest.mark.parametrize("username", ("my_username", None))
@pytest.mark.parametrize("slurm_user", ("test01", None))
async def test_MockCurrentUser_fixture(
MockCurrentUser,
cache_dir,
username,
slurm_user,
):

user_kwargs = dict(username=username)
user_settings_dict = dict(cache_dir=cache_dir, slurm_user=slurm_user)
user_settings_dict = dict(slurm_user=slurm_user)
async with MockCurrentUser(
user_kwargs=user_kwargs, user_settings_dict=user_settings_dict
) as user:
debug(user)
assert user.username == username
assert user.settings.cache_dir == cache_dir
assert user.settings.slurm_user == slurm_user
22 changes: 0 additions & 22 deletions tests/no_version/test_unit_schemas_no_version.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,24 +114,12 @@ def test_user_settings_update():
)
assert update_request_body.slurm_accounts is None

update_request_body = UserSettingsUpdate(cache_dir=None)
assert update_request_body.cache_dir is None
assert "cache_dir" in update_request_body.dict(exclude_unset=True).keys()

update_request_body = UserSettingsUpdateStrict(cache_dir=None)
assert update_request_body.cache_dir is None
assert "cache_dir" in update_request_body.dict(exclude_unset=True).keys()

with pytest.raises(ValidationError):
UserSettingsUpdate(slurm_accounts=[""])
with pytest.raises(ValidationError):
UserSettingsUpdate(slurm_accounts=["a", "a"])
with pytest.raises(ValidationError):
UserSettingsUpdate(slurm_accounts=["a", "a "])
with pytest.raises(ValidationError):
UserSettingsUpdate(cache_dir="non/absolute/path")
with pytest.raises(ValidationError):
UserSettingsUpdate(cache_dir="/invalid;command; $(ls)")
with pytest.raises(ValidationError):
UserSettingsUpdateStrict(ssh_host="NEW_HOST")
with pytest.raises(ValidationError):
Expand All @@ -145,23 +133,13 @@ def test_user_settings_update():
"ssh_tasks_dir",
"ssh_jobs_dir",
"slurm_user",
"cache_dir",
]
nullable_attributes_strict = [
"cache_dir",
]
for key in nullable_attributes:
update_request_body = UserSettingsUpdate(**{key: None})
assert getattr(update_request_body, key) is None
assert key in update_request_body.dict(exclude_unset=True)
assert key not in update_request_body.dict(exclude_none=True)

for key in nullable_attributes_strict:
update_request_body = UserSettingsUpdateStrict(**{key: None})
assert getattr(update_request_body, key) is None
assert key in update_request_body.dict(exclude_unset=True)
assert key not in update_request_body.dict(exclude_none=True)


def test_unit_val_absolute_path():
val_absolute_path("this_attr")("/path")
Expand Down
8 changes: 4 additions & 4 deletions tests/v1/04_api/test_project_apply_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -260,16 +260,16 @@ async def test_project_apply_missing_user_attributes(
):
"""
When using the slurm backend, `user.settings.slurm_user` and
`user.settings.cache_dir` become required attributes.
`user.settings.project_dir` become required attributes.
If they are missing, the apply endpoint fails with a 422 error.
"""

override_settings_factory(FRACTAL_RUNNER_BACKEND="slurm")

async with MockCurrentUser(user_kwargs=dict(is_verified=True)) as user:
# Make sure that user.settings.cache_dir was not set
# Make sure that user.settings.project_dir was not set
debug(user)
assert user.settings.cache_dir is None
assert user.settings.project_dir is None

# Create project, datasets, workflow, task, workflowtask
project = await project_factory(user)
Expand Down Expand Up @@ -303,7 +303,7 @@ async def test_project_apply_missing_user_attributes(
assert res.status_code == 422
assert "User settings are not valid" in res.json()["detail"]

user.settings.cache_dir = "/tmp"
user.settings.project_dir = "/tmp"
user.settings.slurm_user = None
await db.merge(user)
await db.commit()
Expand Down
12 changes: 7 additions & 5 deletions tests/v1/07_full_workflow/test_full_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ async def test_full_workflow(
request.getfixturevalue("relink_python_interpreter_v1")
user_cache_dir = str(tmp777_path / f"user_cache_dir-{backend}")

user_settings_dict["cache_dir"] = user_cache_dir
user_settings_dict["project_dir"] = user_cache_dir
user_settings_dict["slurm_user"] = SLURM_USER

async with MockCurrentUser(
Expand Down Expand Up @@ -274,7 +274,7 @@ async def test_failing_workflow_UnknownError(
request.getfixturevalue("monkey_slurm")
request.getfixturevalue("relink_python_interpreter_v1")
user_cache_dir = str(tmp777_path / f"user_cache_dir-{backend}")
user_settings_dict["cache_dir"] = user_cache_dir
user_settings_dict["project_dir"] = user_cache_dir
user_settings_dict["slurm_user"] = SLURM_USER

async with MockCurrentUser(
Expand Down Expand Up @@ -373,7 +373,7 @@ async def test_failing_workflow_TaskExecutionError(
request.getfixturevalue("monkey_slurm")
request.getfixturevalue("relink_python_interpreter_v1")
user_cache_dir = str(tmp777_path / f"user_cache_dir-{backend}")
user_settings_dict["cache_dir"] = user_cache_dir
user_settings_dict["project_dir"] = user_cache_dir
user_settings_dict["slurm_user"] = SLURM_USER

async with MockCurrentUser(
Expand Down Expand Up @@ -533,7 +533,9 @@ async def test_failing_workflow_JobExecutionError_slurm(

user_cache_dir = str(tmp777_path / "user_cache_dir")
user_kwargs = dict(is_verified=True)
user_settings_dict = dict(cache_dir=user_cache_dir, slurm_user=SLURM_USER)
user_settings_dict = dict(
project_dir=user_cache_dir, slurm_user=SLURM_USER
)
async with MockCurrentUser(
user_kwargs=user_kwargs, user_settings_dict=user_settings_dict
) as user:
Expand Down Expand Up @@ -920,7 +922,7 @@ async def test_non_executable_task_command(
request.getfixturevalue("monkey_slurm")
request.getfixturevalue("relink_python_interpreter_v1")
user_cache_dir = str(tmp777_path / f"user_cache_dir-{backend}")
user_settings_dict["cache_dir"] = user_cache_dir
user_settings_dict["project_dir"] = user_cache_dir
user_settings_dict["slurm_user"] = SLURM_USER

async with MockCurrentUser(
Expand Down
Loading