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

Add cli commands #977

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from
Open

Add cli commands #977

wants to merge 30 commits into from

Conversation

Leahh02
Copy link
Collaborator

@Leahh02 Leahh02 commented Dec 11, 2024

This PR adds cli commands for the curl requests for the ci demonstrator. I implemented it very similarly to the way the requests are implemented in the ci demonstrator, but I can change it to use the requests library, for instance.

@Leahh02 Leahh02 added the WIP (no-ci) Don't run any CI for this PR label Dec 11, 2024
@Leahh02 Leahh02 removed the WIP (no-ci) Don't run any CI for this PR label Jan 7, 2025
@kchilleri kchilleri self-requested a review January 15, 2025 16:16
@kchilleri
Copy link
Collaborator

Tested these commands through Gitlab CI pipeline and they all work as expected. I'm going to think of possible unit tests now and look at the actual code.

@pagrubel do we want this remote stuff to be a part of our documentation?

@Leahh02 Leahh02 added the WIP (no-ci) Don't run any CI for this PR label Jan 23, 2025
@Leahh02
Copy link
Collaborator Author

Leahh02 commented Jan 23, 2025

I added some simple documentation to rest_api.rst, we will discuss the location and content.

I realized I didn't add any error handling into remote_client.py, I will consider adding this.

@Leahh02
Copy link
Collaborator Author

Leahh02 commented Feb 5, 2025

I learned about modules to test fastapi apps: https://fastapi.tiangolo.com/tutorial/testing/ and a module to test typer apps: https://typer.tiangolo.com/tutorial/testing/

@Leahh02 Leahh02 removed the WIP (no-ci) Don't run any CI for this PR label Feb 11, 2025
@@ -53,7 +53,7 @@ python = ">=3.8.3,<=3.13.0"

# Package dependencies
Flask = { version = "^2.0" }
fastapi = { version = "0.109.2" }
fastapi = "0.115.8"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering why we are upgrading to this version?

I think we should use a range of versions (e.g. fastapi = "^0.109.2") instead of a specific version. This will avoid overly constraining dependencies and conflicts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is more of a team discussion, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ this will be taken care of in a separate PR.

@kchilleri
Copy link
Collaborator

@Leahh02 is there any way you can add a remote command for beeflow core status?

Thankfully, this has already been added to remote.py, in def get_core_status(), and you'd only have to replace this curl request:

curl ssh_target:port/core/status/

I think this would be very useful to have when interacting remotely because we'll be able to check that all the appropriate components are up and running.

@Leahh02
Copy link
Collaborator Author

Leahh02 commented Feb 25, 2025

@Leahh02 is there any way you can add a remote command for beeflow core status?

Thankfully, this has already been added to remote.py, in def get_core_status(), and you'd only have to replace this curl request:

curl ssh_target:port/core/status/

I think this would be very useful to have when interacting remotely because we'll be able to check that all the appropriate components are up and running.

I can absolutely do that!

@Leahh02
Copy link
Collaborator Author

Leahh02 commented Feb 25, 2025

@Leahh02 is there any way you can add a remote command for beeflow core status?
Thankfully, this has already been added to remote.py, in def get_core_status(), and you'd only have to replace this curl request:

curl ssh_target:port/core/status/

I think this would be very useful to have when interacting remotely because we'll be able to check that all the appropriate components are up and running.

I can absolutely do that!

My most recent commit adds this in along with a test. This was a quick fix, it just took me a while to do it because I had to finish other stuff up first :)

Copy link
Collaborator

@kchilleri kchilleri left a comment

Choose a reason for hiding this comment

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

These remote commands have been tested both locally and within a Gitlab CI pipeline. Everything seems to work well. Great job!

@Leahh02 Leahh02 mentioned this pull request Feb 27, 2025
2 tasks
@kchilleri kchilleri requested a review from arhall0 February 27, 2025 17:48

try:
droppoint_result = subprocess.run(
["jq", "-r", ".droppoint", "droppoint.env"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably just be done with the pythonjson package. Otherwise this has OS issues.



@app.command()
def copy(file_path: pathlib.Path = typer.Argument(..., help="path to copy to droppoint")):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This currently doesn't work for copying from local to remote.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, let me test it again locally (outside of Gitlab CI pipeline)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be fixed now to work locally. I don't know how to test it to see if it still works with the Gitlab CI pipeline though. I'll update the unit tests now

@arhall0
Copy link
Collaborator

arhall0 commented Feb 27, 2025

I set up BEE on remote by making sure my config had remote_api = True and that I had a unique remote_api_port. Then I did beeflow core start

On my local machine I made sure to set remote_api_port to the same port as remote. beeflow core start is unnecessary for local.

I checked each command from local.

  • beeflow remote connection, beeflow remote droppoint, beeflow remote core-status all worked without issue
  • beeflow remote submit worked locally after manually setting up the files on remote. From ~/.beeflow/droppoint:
    -- beeflow package $BEE_PATH/examples/cat-grep-tar .
    -- mkdir cat-grep-tar-workdir
    -- cp $BEE_PATH/examples/cat-grep-tar/lorem.txt cat-grep-tar-workdir/

@Leahh02 Leahh02 added the WIP (no-ci) Don't run any CI for this PR label Feb 27, 2025
check=True
)
droppoint_path = droppoint_result.stdout.strip()
os.makedirs(droppoint_path, exist_ok=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This command is making the directory locally. But also, the directory should exist on remote if remote was setup properly.

This errors for me because I can't make the folder locally because of permissions issues. If I comment this out, it works as expected.

@Leahh02 Leahh02 removed the WIP (no-ci) Don't run any CI for this PR label Feb 27, 2025
@arhall0
Copy link
Collaborator

arhall0 commented Feb 28, 2025

After the most recent commits I can run a workflow end-to-end from local on remote. For cat-grep-tar these are the commands:

beeflow remote droppoint $SSH_TARGET
beeflow package $BEE_PATH/examples/cat-grep-tar .
beeflow remote copy $USER $SSH_TARGET cat-grep-tar.tgz
mkdir cat-grep-tar-workdir
cp $BEE_PATH/examples/cat-grep-tar/lorem.txt cat-grep-tar-workdir
beeflow remote copy $USER $SSH_TARGET cat-grep-tar-workdir
beeflow remote submit $SSH_TARGET test_remote cat-grep-tar.tgz workflow.cwl input.yml

I think that this works well. It's maybe a bit verbose so maybe there are things we could do in the future to improve the workflow.

Another improvement for the future would be a way to check the status of your job & pull down the results to local. Automating these things to as few as commands as possible would improve their utility over just using things like rsync and scp.

@arhall0
Copy link
Collaborator

arhall0 commented Feb 28, 2025

In general test coverage is good. For remote_client.py, test coverage is missing mostly for lines which are checking for errors or exceptions:

--------- coverage: platform darwin, python 3.10.13-final-0 ----------
Name                                                  Stmts   Miss  Cover   Missing
-----------------------------------------------------------------------------------
beeflow/client/remote_client.py                          80     60    25%   22, 27, 36-48, 54-71, 79-102, 112-141, 147-159

This is probably not a big deal.

I also noticed the droppoint folder is created when you run tests. This also probably doesn't matter too much.

@kchilleri
Copy link
Collaborator

I still need to test these new changes on the Gitlab CI work, please don't merge yet!

@Leahh02 Leahh02 added the WIP (no-ci) Don't run any CI for this PR label Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP (no-ci) Don't run any CI for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants