Skip to content

Conversation

@niklastheman
Copy link
Collaborator

  • Session validation change
  • API client - start session provides seed_model_id

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses validation issues in session objects by ensuring that boolean fields are correctly interpreted and modifying session name validation. Additionally, the API client has been updated to provide a seed_model_id when starting a session.

  • Change the validation checks for delete_models_storage and validate to explicitly check for None.
  • Update the session DTO’s name field to be Optional and remove its validator.
  • Include seed_model_id in the API client’s session start payload.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
fedn/network/storage/statestore/stores/dto/session.py Updated boolean validations; modified the name field to Optional and removed its validator.
fedn/network/api/client.py Added seed_model_id to the JSON payload for starting sessions.
Comments suppressed due to low confidence (2)

fedn/network/storage/statestore/stores/dto/session.py:58

  • Removal of the name validator may conflict with the PR title which suggests that a session should be invalid without a name. Consider reintroducing a validation check for the name field if it is indeed required.
@validator def validate(self): if not self.name: raise ValidationError("name", "Name is required")

fedn/network/api/client.py:660

  • [nitpick] The use of the variable 'model_id' for the 'seed_model_id' key can be confusing. Consider renaming the variable to 'seed_model_id' to maintain consistency.
"seed_model_id": model_id,

@niklastheman niklastheman merged commit 8f65507 into master Apr 23, 2025
23 checks passed
@niklastheman niklastheman deleted the bug/SK-1571 branch April 23, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants