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

De-register api command from Ersilia CLI #1289

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

adebisi4145
Copy link
Contributor

@adebisi4145 adebisi4145 commented Oct 3, 2024

Thank you for taking your time to contribute to Ersilia, just a few checks before we proceed

  • Have you followed the guidelines in our Contribution Guide
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Description

There's a command line api in Ersilia that calls different APIs like run, calculate and so on. Ersilia no longer allow this, so the command is deprecated and needs to be deleted from Ersilia CLI

Changes to be made

  1. Run a model to ensure I have a functional installation of Ersilia
  2. Create a branch and switch to the branch
  3. Remove the api command from the create_cli.py script
  4. Remove the implementation of this command
  5. Run a model as before using the ersilia run command.
  6. Check that the CLI does not crash by running ersilia api run... and that there's an error message

Status

  • I ran a model using ersilia api run here
  • I created a branch called DeregisterApi and switched to it
  • I removed the cmd.api() command from the create_cli.py script here
  • I removed the implementation of this command in cmd.py, api.py and deleted api.py file
  • I successfuly ran the same model as before using the ersilia run command.
  • I ran the model again using ersilia api run... , the CLI did not crash and I also got an error message here

Is this pull request related to any open issue? If yes, replace issueID below with the issue ID

Related to #1263

@adebisi4145
Copy link
Contributor Author

adebisi4145 commented Oct 3, 2024

I look forward to your feedback @DhanshreeA . Thank you for your time

@P-Mefut
Copy link

P-Mefut commented Oct 3, 2024

hi @adebisi4145, if your removing the command, shouldn't the command file api.py also be removed?

@adebisi4145
Copy link
Contributor Author

adebisi4145 commented Oct 3, 2024

hi @adebisi4145, if your removing the command, shouldn't the command file api.py also be removed?

Hello @P-Mefut , I'm currently working on that and trying to decide whether I should remove the api.py file or I should just remove all the lines of code. I really appreciate your feedback on this.

@P-Mefut
Copy link

P-Mefut commented Oct 4, 2024

Well if the command is no longer required then it's best to remove the api.py file... but I prefer we wait on @DhanshreeA to shed more light on it!!!

@DhanshreeA
Copy link
Contributor

Well if the command is no longer required then it's best to remove the api.py file... but I prefer we wait on @DhanshreeA to shed more light on it!!!

Exactly, thanks @P-Mefut. Please take care of this @adebisi4145 :) Thank you for your inputs, both.

@@ -1,49 +0,0 @@
import click
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the file entirely.

Copy link
Contributor Author

@adebisi4145 adebisi4145 Oct 5, 2024

Choose a reason for hiding this comment

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

Thank you @DhanshreeA for the feedback, this has now been done

@adebisi4145
Copy link
Contributor Author

Exactly, thanks @P-Mefut. Please take care of this @adebisi4145 :) Thank you for your inputs, both.

Duly note @DhanshreeA , working on that immediately

@Malikbadmus
Copy link
Contributor

Malikbadmus commented Oct 6, 2024

Great work @adebisi4145 .

@DhanshreeA , I've reviewed and ran several tests, and this works as intended.

@adebisi4145
Copy link
Contributor Author

Great work @adebisi4145 , I've reviewed and ran several tests, and this works as intended.

Thank you @Malikbadmus for taking the time to review and test my code

@DhanshreeA DhanshreeA merged commit 56aaaa8 into ersilia-os:master Oct 8, 2024
18 checks passed
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.

4 participants