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

Remove GoogleDrivePath Dead Code #528

Merged
merged 32 commits into from
May 13, 2022
Merged

Conversation

srivarra
Copy link
Contributor

@srivarra srivarra commented Apr 28, 2022

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

Closes #512. Removes all references to GoogleDrivePath.

How did you implement your changes

All use cases of the Google Drive API in ark-analysis.

  • Google Python Packages in the requirements.txt and in setup.py.

    • data_utils.py
    • Imports GoogleDrivePath, drive_write_out, path_join
  • deepcell_service_utils.py

    • Imports GoogleDrivePath, drive_write_out, path_join, DriveOpen
  • google_drive_utils_test.py

    • Delete it
  • google_drive_utils.py

    • Delete it
  • io_utils

  • load_utils.py

  • segmentation_utils.py

    • drive_write_out and path_join
  • conf.py

    • Remove Google packages
  • index.rst

    • Remove GoogleDrive references
  • google_drive_usage.md

    • Remove file
  • Segment_Image_Data.ipynb

    • Uses deepcell_service_utils.py.
  • Remove Google API tokens

  • Successfully building the Docker Image

Remaining issues

None ATM.

@srivarra srivarra assigned srivarra and unassigned srivarra Apr 29, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

There are a ton of stylistic changes in the PR. I'm not necessarily opposed to them, but we should come to a general consensus first about what patterns we're trying to encourage. Based on looking at the first file, I'm not quite grasping what the theme is for the changes being made.

This also makes it hard to identify which code changes are related to purpose of the PR

ark/utils/data_utils.py Outdated Show resolved Hide resolved
ark/utils/data_utils.py Outdated Show resolved Hide resolved
ark/utils/data_utils.py Outdated Show resolved Hide resolved
ark/utils/data_utils.py Outdated Show resolved Hide resolved
ark/utils/data_utils.py Outdated Show resolved Hide resolved
@srivarra
Copy link
Contributor Author

Oh, that wasn't intended, I think my formatter may have done it.

Copy link
Contributor

@ackagel ackagel left a comment

Choose a reason for hiding this comment

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

looks good. One more cleanup, there should be a folder called .toks which can be removed. It held api relevant info like the encrypted google cloud api token.

@srivarra
Copy link
Contributor Author

@ackagel Should I also remove mentions of .toks in the Dockerfile and in start_docker.sh?

@srivarra srivarra requested a review from ngreenwald May 12, 2022 00:44
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks great! Will you confirm that there's no issues with the dockerfile by changing dockerhub from on tag to on commit and then pulling the latest image? You can check out @alex-l-kong's last commit on the pixel clustering PR to see, or ask him for help at standup today

docs/index.rst Outdated Show resolved Hide resolved
templates_ark/Segment_Image_Data.ipynb Outdated Show resolved Hide resolved
@srivarra
Copy link
Contributor Author

@ngreenwald The docker builds successfully.

@srivarra srivarra requested a review from ngreenwald May 13, 2022 21:15
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Awesome, looks good

@ngreenwald ngreenwald merged commit a84cf4d into master May 13, 2022
@ngreenwald ngreenwald deleted the remove_googledrivepath_dead_code branch May 13, 2022 21:55
@srivarra srivarra added enhancement New feature or request dependencies Pull requests that update a dependency file and removed enhancement New feature or request labels Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove 'GoogleDrivePath' dead code
3 participants