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

Merged
merged 40 commits into from
Apr 1, 2025
Merged

Add cli commands #977

merged 40 commits into from
Apr 1, 2025

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
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
@Leahh02 Leahh02 removed the WIP (no-ci) Don't run any CI for this PR label Mar 26, 2025
@Leahh02 Leahh02 added WIP (no-ci) Don't run any CI for this PR and removed WIP (no-ci) Don't run any CI for this PR labels Mar 26, 2025
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.

Besides the requested changes for adding a bit more error checking everything else looks good to me!

sys.exit(1)

try:
with open("droppoint.env", "r", encoding="utf-8") as f:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When reviewing this command, I forgot to run the beeflow remote droppoint command that creates the droppoint.env file. This gave me a "FileNotFoundError" error. I think it would be helpful to add an exception, something like:
except FileNotFoundError:
warn("Error: File droppoint.env not found. Did you run 'beeflow remote droppoint $ssh_target' to create this file?")

@kchilleri kchilleri merged commit 7168103 into develop Apr 1, 2025
6 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.

3 participants