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

Bugfix/SK-1365 | Fix so server functions code resets by sessions #806

Merged
merged 8 commits into from
Feb 4, 2025

Conversation

viktorvaladi
Copy link
Contributor

@viktorvaladi viktorvaladi commented Jan 31, 2025

In the current implementation, the code for server functions would not reset between sessions.

Also fixed in this PR so that users can use "print" in their code and it will be outputted in the dashboard logs.

@github-actions github-actions bot added the minor label Feb 3, 2025
@viktorvaladi viktorvaladi requested a review from Wrede February 3, 2025 13:56
@@ -6,9 +6,10 @@
import fedn.network.grpc.fedn_pb2 as fedn
import fedn.network.grpc.fedn_pb2_grpc as rpc
from fedn.common.log_config import logger
from fedn.network.combiner.hooks.allowed_import import * # noqa: F403
Copy link
Member

Choose a reason for hiding this comment

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

går det inte att bara köra "import fedn.....allowed_imports" alltså skippa from så du inte behöver *?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Borde funka, testar


# imports for user code
from fedn.network.combiner.hooks.allowed_import import Dict, List, ServerFunctionsBase, Tuple, np, random # noqa: F401
from fedn.network.combiner.hooks.allowed_import import ServerFunctionsBase # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

varför noqa? och varför måste du dela upp imports från samma modul?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

behöver inte noqa, tar bort den

Dela upp från samma modul är bara för det blir snyggare med typningen så att IDE'n ser att det är en ServerFuctionsBase vi jobbar med

Copy link
Member

@Wrede Wrede left a comment

Choose a reason for hiding this comment

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

Bra, men vill att Stefan kollar på print

@stefanhellander
Copy link
Collaborator

Har användarens kod tillgång till någon känslig information, konfigurationsdata, miljövariabler osv?

@viktorvaladi
Copy link
Contributor Author

Har användarens kod tillgång till någon känslig information, konfigurationsdata, miljövariabler osv?

Den borde inte ha det. Inga känsliga env vars som exponeras i deploymenten och den körs i en egen kernel med automountServiceAccountToken: false och andra inställningar som hindrar k8 escalations osv. Se server-functions-deployment.yaml i studio repot. Modellerna streamas från combinern hit så ingen db access med

@stefanhellander
Copy link
Collaborator

Har användarens kod tillgång till någon känslig information, konfigurationsdata, miljövariabler osv?

Den borde inte ha det. Inga känsliga env vars som exponeras i deploymenten och den körs i en egen kernel med automountServiceAccountToken: false och andra inställningar som hindrar k8 escalations osv. Se server-functions-deployment.yaml i studio repot. Modellerna streamas från combinern hit så ingen db access med

I sådana fall ser jag inte att det skulle vara några problem med detta, OK att merga för min del.

@Wrede Wrede merged commit f82ce75 into master Feb 4, 2025
17 checks passed
@Wrede Wrede deleted the bugfix/SK-1365 branch February 4, 2025 15: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