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

Feature/SK-805 | FEDn cli - new order #593

Merged
merged 14 commits into from
May 7, 2024
Merged

Conversation

niklastheman
Copy link
Contributor

Some changes to the CLI

  • fedn run combiner -> fedn combiner start
  • fedn run client -> fedn client start
  • fedn client/combiner/model/package/round/session/status/validation list - list objects from statestore
  • refactoring - refactored code into separate files
  • fedn config - lists env:s (and values) relevant for the cli

Copy link
Member

@ahellander ahellander left a comment

Choose a reason for hiding this comment

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

It looks right to me. I think you will have some conflicts when you merge in master though, so need to check that.

I think it would be good if the "config" commmand also has one for actually configuring. I.e. prompt user for key configurations. But that could be a separate PR perhaps.

@@ -19,6 +19,17 @@ However, during development of a new model it will be necessary to reinitialize.

2. Restart the clients.

Q: Can I skip fetching the remote package and instead use a local folder when developing the compute package
Copy link
Member

Choose a reason for hiding this comment

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

I think I removed this Q in current master (since --remote False flag currently not working)

@niklastheman
Copy link
Contributor Author

It looks right to me. I think you will have some conflicts when you merge in master though, so need to check that.

I think it would be good if the "config" commmand also has one for actually configuring. I.e. prompt user for key configurations. But that could be a separate PR perhaps.

  • Merged
  • I actually tried creating a command to set the env:s interactivity but found no way to make it work. The best solution I found was to prompt the user and the return the strings to paste in to a terminal. This felt sub optimal though so left it out. If you want it I can easily re-implement it.

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.

Looks nice! Just think we should add a "obsolete" warning or error to the old "fedn run client/combiner", since this is a major change and will break many container images. Best would be to actually allow "run client/combiner" for a while and instead pass all arguments to the new click command, that way we can have a warning between two major releases, and then in the future update to am error.

@niklastheman
Copy link
Contributor Author

Looks nice! Just think we should add a "obsolete" warning or error to the old "fedn run client/combiner", since this is a major change and will break many container images. Best would be to actually allow "run client/combiner" for a while and instead pass all arguments to the new click command, that way we can have a warning between two major releases, and then in the future update to am error.

deprecation warning added

@niklastheman niklastheman requested a review from Wrede May 7, 2024 07:42
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.

Hold?

@niklastheman
Copy link
Contributor Author

Hold?

Sure! 👌 Can be merged prior to next release.

@niklastheman niklastheman merged commit a25ac0c into master May 7, 2024
11 checks passed
@niklastheman niklastheman deleted the feature/SK-805 branch May 7, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants