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 asl module lib #452

Merged
merged 1 commit into from
May 1, 2024
Merged

Add asl module lib #452

merged 1 commit into from
May 1, 2024

Conversation

BenoitDherin
Copy link
Collaborator

This PR adds the folder asl at the top level directory to hold our code utils (e.g., convenients wrappers around GCP API calls). For now, it contains only a single file lib.py with two functions:

  • upload_directory_to_gcs that upload a local folder to a bucket
  • download_directory_from_gcs that download the content of a bucket path to a local folder

The setup has been modified (Makefile, requirements.txt, plus an additional setup.py) so that now make install has for result that the asl module can be imported directly in any notebook after make install is ran. If the code in the ./asl folder is modified, there should not be a need for re-runing make isntall (because of the pip install -e . in the Makefile). Here is an example how to import the asl module in the notebook:

image

A test folder named tests has been added as placeholder for the unit tests for the asl module. Now make tests triggers the tests.

@takumiohym
Copy link
Collaborator

@BenoitDherin That's a nice idea to add lib for reusable small util functions.

In terms of the functions you added, could you share more context on when we use them?
For directory copy, I usually use gsutil cp -r simply and haven't met situations where it is not enough.

@sanjanalreddy
Copy link
Collaborator

sanjanalreddy commented Apr 29, 2024

@BenoitDherin the PR is failing the pre-commit, except for that the code looks good to me! Once the pre-commit check passes, you can merge.

@takumiohym
Copy link
Collaborator

pre-commit failure is fixed in this PR. #454
If you want to confirm, you can rebasing on the current master branch. (Or you can just ignore the error.)

@BenoitDherin BenoitDherin merged commit b5001da into master May 1, 2024
4 of 5 checks passed
@BenoitDherin BenoitDherin deleted the asl-lib-dev branch May 1, 2024 15:55
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