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

FIX: skip minification tests on M1/M2 macs and run them otherwise #497

Merged
merged 6 commits into from
Jan 16, 2023

Conversation

kaczmarj
Copy link
Collaborator

This adds back in the minification tests to CI. These tests passed on my Debian Linux machine (intel 64-bit). See docker/for-mac#5191 for an explanation of why ptrace does not work on arm macs.

This adds back in the minification tests to CI. These tests passed on my
Debian Linux machine (intel 64-bit). See
docker/for-mac#5191 for an explanation of
why ptrace does not work on arm macs.
@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Base: 83.67% // Head: 88.60% // Increases project coverage by +4.93% 🎉

Coverage data is based on head (927b960) compared to base (95f8f91).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
+ Coverage   83.67%   88.60%   +4.93%     
==========================================
  Files          11       11              
  Lines        1017     1027      +10     
==========================================
+ Hits          851      910      +59     
+ Misses        166      117      -49     
Impacted Files Coverage Δ
neurodocker/cli/cli.py 90.00% <100.00%> (-10.00%) ⬇️
neurodocker/cli/minify/trace.py 83.90% <0.00%> (+58.62%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kaczmarj
Copy link
Collaborator Author

@satra - is this change ok with you? i use the following code to test if we are running on an m1/m2 mac:

import platform

def _arm_on_mac():
    """Return True if on an ARM processor (M1/M2) in macos operating system."""
    is_mac = platform.system().lower() == "darwin"
    is_arm = platform.processor().lower() == "arm"
    return is_mac and is_arm

@satra
Copy link
Contributor

satra commented Jan 16, 2023

this PR should technically update, the CLI as well. see https://github.com/ReproNim/neurodocker/blob/master/neurodocker/cli/cli.py#L13 and also later in that file. and perhaps it should use the same function to determine if it's available.

also we should consider checking if trace can be carried out inside a singularity instance with a mounted mamba+reprozip. (not for this PR).

@kaczmarj
Copy link
Collaborator Author

this PR should technically update, the CLI as well. see https://github.com/ReproNim/neurodocker/blob/master/neurodocker/cli/cli.py#L13 and also later in that file. and perhaps it should use the same function to determine if it's available.

i added that, so neurodocker minify is not available on arm-based macs.

@kaczmarj
Copy link
Collaborator Author

also we should consider checking if trace can be carried out inside a singularity instance with a mounted mamba+reprozip. (not for this PR).

interesting idea. how would we save the minified singularity container? perhaps we could copy the minified directories into some tarball, and then inject them into a new container?

@satra
Copy link
Contributor

satra commented Jan 16, 2023

how would we save the minified singularity container? perhaps we could copy the minified directories into some tarball, and then inject them into a new container?

yup, something like that.

@kaczmarj
Copy link
Collaborator Author

@satra - ready to merge on my end. let me know if you want any other changes.

@satra
Copy link
Contributor

satra commented Jan 16, 2023

lgtm. you can decide which PR merge should trigger a release: add release label and minor or patch depending on the type of release.

if you are not planning to merge anymore PRs yet i would suggest labeling this one with release + patch and merging.

@kaczmarj kaczmarj added release to trigger release from pr patch patch tag for release labels Jan 16, 2023
@kaczmarj kaczmarj merged commit b881469 into master Jan 16, 2023
@kaczmarj kaczmarj deleted the fix/skip-minify-tests-arm-mac branch January 16, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch patch tag for release release to trigger release from pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants