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

[Cloud Deployment IVc] Deploy NeuroConv in AWS with EFS #1086

Merged
merged 65 commits into from
Dec 6, 2024

Conversation

CodyCBakerPhD
Copy link
Member

The most important step in the process, combining each of the previous developments

@CodyCBakerPhD CodyCBakerPhD self-assigned this Sep 16, 2024
@CodyCBakerPhD
Copy link
Member Author

CodyCBakerPhD commented Sep 29, 2024

@h-mayorquin This one is ready too!

Proof of key tests passing: https://github.com/catalystneuro/neuroconv/actions/runs/11089506338/job/30810706117?pr=1086

Of course, the data is just sitting on the EFS until #1089 allows direct upload to DANDI. See better idea https://github.com/catalystneuro/neuroconv/pull/1086/files#diff-fbec866f95a8edcf190ce00113773906f58d29fdf491a2c27dbf57dedecf9187R183 that I did not have time to figure a way to do (probably another small helper function and CLI entrypoint to be called from the same neuroconv docker image)

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review September 29, 2024 04:18
Comment on lines +123 to +130
available_efs_volumes = efs_client.describe_file_systems()
matching_efs_volumes = [
file_system
for file_system in available_efs_volumes["FileSystems"]
for tag in file_system["Tags"]
if tag["Key"] == "Name" and tag["Value"] == efs_volume_name
]
max_iterations = 10
Copy link
Member Author

@CodyCBakerPhD CodyCBakerPhD Sep 29, 2024

Choose a reason for hiding this comment

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

This middle-man EFS management is rather annoying but had bugs with duplicates otherwise. Maybe a better way in the future would be direct EFS ID passing that overrides the 'search' based creation based on name pattern

Comment on lines +214 to +218
# Cleanup EFS after job is complete - must clear mount targets first, then wait before deleting the volume
efs_volumes = efs_client.describe_file_systems()
matching_efs_volumes = [
file_system
for file_system in efs_volumes["FileSystems"]
Copy link
Member Author

Choose a reason for hiding this comment

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

@h-mayorquin Also, in a follow-up, you may wish to add logic for detecting if the EFS volume exists before this method is called, in which case we might not want to clean it up automatically (might be shared across jobs or something)

And/or always control the behavior with an extra argument cleanup: bool perhaps. Given the billing consequences I tend to err on the side of always perform cleanup, hence the current logic

Base automatically changed from rclone_aws to main November 28, 2024 01:18
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

Merging this to have this done.

@h-mayorquin h-mayorquin merged commit 96dfdff into main Dec 6, 2024
40 checks passed
@h-mayorquin h-mayorquin deleted the neuroconv_aws branch December 6, 2024 21:02
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.69%. Comparing base (c4afad3) to head (cfe9ade).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...on_specification/_yaml_conversion_specification.py 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1086   +/-   ##
=======================================
  Coverage   90.69%   90.69%           
=======================================
  Files         129      129           
  Lines        8186     8189    +3     
=======================================
+ Hits         7424     7427    +3     
  Misses        762      762           
Flag Coverage Δ
unittests 90.69% <75.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...on_specification/_yaml_conversion_specification.py 97.26% <75.00%> (+0.11%) ⬆️

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.

2 participants