-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add cli commands #977
Conversation
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? |
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. |
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/ |
beeflow/client/remote_client.py
Outdated
check=True | ||
) | ||
droppoint_path = droppoint_result.stdout.strip() | ||
os.makedirs(droppoint_path, exist_ok=True) |
There was a problem hiding this comment.
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.
After the most recent commits I can run a workflow end-to-end from local on remote. For 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 |
In general test coverage is good. For --------- 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 |
I still need to test these new changes on the Gitlab CI work, please don't merge yet! |
There was a problem hiding this 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: |
There was a problem hiding this comment.
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?")
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.