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 IVb] Rclone in AWS on EFS #1085

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

CodyCBakerPhD
Copy link
Member

@CodyCBakerPhD CodyCBakerPhD commented Sep 16, 2024

@pauladkisson Can you setup your Google Drive in accordance with these instructions? (just copy over https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/spikeglx/Noise4Sam_g0 as shown) let me know when that's ready so I can try to deploy the CI for this

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

@pauladkisson And while you're at it, could you include the phy folder in anticipating of using the test specification files? https://github.com/catalystneuro/neuroconv/blob/main/tests/test_on_data/test_yaml/conversion_specifications/GIN_conversion_specification.yml#L53

@pauladkisson
Copy link
Member

@pauladkisson Can you setup your Google Drive in accordance with these instructions? (just copy over https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/spikeglx/Noise4Sam_g0 as shown) let me know when that's ready so I can try to deploy the CI for this

Done!

@pauladkisson
Copy link
Member

@pauladkisson And while you're at it, could you include the phy folder in anticipating of using the test specification files? https://github.com/catalystneuro/neuroconv/blob/main/tests/test_on_data/test_yaml/conversion_specifications/GIN_conversion_specification.yml#L53

Where exactly does the phy folder go? Under testing_rclone_spikeglx/ci_tests? Or somewhere else?

@CodyCBakerPhD
Copy link
Member Author

Where exactly does the phy folder go? Under testing_rclone_spikeglx/ci_tests? Or somewhere else?

Good question - actually, in light of that, I've modified the structure shown in the docstring

Sorry about that, could you change it when you get a chance?

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review September 28, 2024 16:24
@CodyCBakerPhD
Copy link
Member Author

Comment on lines +174 to +176
container_overrides["environment"] = [
{"name": key, "value": value} for key, value in environment_variables.items()
]
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to fix this, was not tested before now

Comment on lines +535 to +539

# AWS Batch does not allow colons, slashes, or periods in job definition names
parsed_docker_image_name = str(docker_image)
for disallowed_character in [":", r"/", "."]:
parsed_docker_image_name = parsed_docker_image_name.replace(disallowed_character, "-")
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out these are really restrictive, had a fix this using a GHCR image source

@@ -540,7 +546,6 @@ def _generate_job_definition_name(
job_definition_name += f"_{efs_id}"
if docker_tag is None or docker_tag == "latest":
date = datetime.now().strftime("%Y-%m-%d")
job_definition_name += f"_created-on-{date}"
Copy link
Member Author

Choose a reason for hiding this comment

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

After internal debate, I decided to roll this back since even latest images will be pulled, not at time of job definition declaration, but rather at time of instance run (which could be well after)

Should reduce the amount of spam for the normal AWS tests

@CodyCBakerPhD
Copy link
Member Author

@h-mayorquin Also leaving this as a note:

You may want to include job definition cleanup in the tearDown step. I'm not for the time being because there is no cost and it contains provenance with very useful debug info (and the basic non-EFS test should only create one for all time, should even theoretically re-use without making new revisions). But EFS mounting is a very important association with it so there is technically one job definition per EFS related test

Or you can just clean them up in a separate task periodically (monthly?), it's only a couple lines of boto3 to find the names that match some pattern and deregister them

Also, there is a rare condition where if the test is interrupted via GitHub (therefore not triggering the natural tearDown) the EFS might not get cleaned up - I'm not worried about cost since even if you forget about it for these tests for weeks it'll still only be pennies (though you should check it from time to time just to be sure) - but instead the test assertion errors out due to multiple copies of EFS of the same name, so if this becomes a common annoyance I recommend adding a pre-step in the test case setup to clean any existing EFS of the expected name

Copy link

codecov bot commented Sep 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.58%. Comparing base (36464df) to head (9ea3bab).
Report is 19 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   90.44%   90.58%   +0.13%     
==========================================
  Files         129      129              
  Lines        8055     8164     +109     
==========================================
+ Hits         7285     7395     +110     
+ Misses        770      769       -1     
Flag Coverage Δ
unittests 90.58% <ø> (+0.13%) ⬆️

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

see 2 files with indirect coverage changes

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