Fmriprep: Add failsafe version detection, to prevent CLI crash

[adelavega]

When building an fmriprep image directly from github, the version is not set correctly, as defaults to ''.

As a consequence the CLI crashes, because it can't parse the version:

Traceback (most recent call last):
  File "/usr/local/miniconda/bin/fmriprep", line 10, in <module>
    sys.exit(main())
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 312, in main
    opts = get_parser().parse_args()
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 50, in get_parser
    currentv = Version(__version__)
  File "/usr/local/miniconda/lib/python3.7/site-packages/packaging/version.py", line 221, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
packaging.version.InvalidVersion: Invalid version: ''

I suggest a failsafe unknown version is set if the version is an empty string.

[oesteban]

To consider in conjunction with #1779

[effigies]

What about 'stable'?

[oesteban]

When building an fmriprep image directly from github, the version is not set correctly, as defaults to ''.

What did you exactly try? I'll need to replicate to understand what is going on. Only now I understand why you proposed this https://github.com/poldracklab/fmriprep/pull/1829/files#diff-b93ab0122f4b2ac6738b896651cac6ccR49-R50

This apparently simple issue might be masking a deeper problem with versioneer and our version tracking infrastructure.

[adelavega]

I believe that I git cloned this repository, cd fmriprep and docker build . -t fmriprep.

Then when I did docker run there version was an empty string.

[oesteban]

Okay, then I guess you should've passed the VERSION --build-arg:

https://github.com/poldracklab/fmriprep/blob/2e5a95fc3643f3507209bad18407757989f63c5e/.circleci/config.yml#L61-L67

Can you try the following?

docker build --rm -t fmriprep \
    --build-arg BUILD_DATE=`date -u +"%Y-%m-%dT%H:%M:%SZ"` \
    --build-arg VCS_REF=`git rev-parse --short HEAD` \
    --build-arg VERSION=$( python get_version.py ) .

We probably want to have a Makefile with a build target that includes the build args.

[effigies]

I think the point was just that it's a minor pain and we can probably set it up not to trip people up who don't want to learn about build args just to run a test.

[adelavega]

Ah yeah. My internet is too crappy right now to try it, but I'm sure that works.

I agree with @effigies here. Maybe if build-args are missing, it should either throw an error at build time, or (better) throw a warning, but default to a value that is valid (if meaningless) later on.

[emdupre]

I think we tried to update the docs for this in #1797, so it'd be great to review if there's more that should be added there, too.

And just explicitly: I'm a strong +1 on throwing a more descriptive error !

[adelavega]

Ah, nice. How ironic that I had that very issue for the docusprint.

Docs look good to me. Unfortunately, there's a very good chance I probably wouldn't have read the docs anyways, and there's too many people like me in this world.