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

MNT: Update Docker pins for Python packages #2372

Closed
wants to merge 2 commits into from

Conversation

effigies
Copy link
Member

Fixes #2351.

@tsalo
Copy link
Collaborator

tsalo commented Feb 24, 2021

Here's the error I'm seeing in the ds* tests:

Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/plugins/legacymultiproc.py", line 67, in run_node
    result["result"] = node.run(updatehash=updatehash)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 516, in run
    result = self._run_interface(execute=True)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 635, in _run_interface
    return self._run_command(execute)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 741, in _run_command
    result = self._interface.run(cwd=outdir)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/interfaces/freesurfer/base.py", line 149, in run
    return super(FSCommand, self).run(**inputs)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/interfaces/base/core.py", line 435, in run
    runtime = self._post_run_hook(runtime)
  File "/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/interfaces/freesurfer.py", line 230, in _post_run_hook
    runtime = super(TruncateLTA, self)._post_run_hook(runtime)
  File "/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/interfaces/registration.py", line 331, in _post_run_hook
    return super(MRICoregRPT, self)._post_run_hook(runtime)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/interfaces/mixins/reporting.py", line 50, in _post_run_hook
    self._generate_report()
  File "/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/interfaces/report_base.py", line 90, in _generate_report
    out_file=self._out_report,
  File "/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/viz/utils.py", line 361, in compose_view
    out_file.write_text("\n".join(_compose_view(bg_svgs, fg_svgs, ref=ref)))
  File "/usr/local/miniconda/lib/python3.7/site-packages/niworkflows/viz/utils.py", line 396, in _compose_view
    r.moveto(0, yoffset, scale=scales[i])
TypeError: moveto() got an unexpected keyword argument 'scale'

I think that comes from this niworkflows line, which I think is broken due to a change in svgutils. Specifically, in btel/svg_utils#55 (tied to release 0.3.2 this January), they removed the scale argument in favor of scale_x and scale_y.

EDIT: @effigies I see you already caught that in nipreps/niworkflows#595. Perhaps the problem now is that, since niworkflows requires a version that is not 0.3.2, and there is now a 0.3.4 release... niworkflows is installing a newer version that still has the same problem?
EDIT2: There is a fix for that in nipreps/niworkflows#620, so I guess we just need a niworkflows release before this will work.

@tsalo
Copy link
Collaborator

tsalo commented Mar 3, 2021

@effigies @oesteban is there anything blocking a niworkflows release? Would it be possible to make one soon-ish?

@mgxd
Copy link
Collaborator

mgxd commented Mar 3, 2021

@tsalo dunno if you've seen, but there are a few RCs available - https://github.com/nipreps/niworkflows/tree/1.4.0rc5

@tsalo
Copy link
Collaborator

tsalo commented Mar 3, 2021

@mgxd I had not seen that, thanks! Would it be reasonable to pin to a release candidate in fMRIPrep?

@mgxd
Copy link
Collaborator

mgxd commented Mar 3, 2021

I think we discussed and agreed on pinning the development branch (in this case, master) to the heads of our niprep dependencies (niworkflows, sdcflows, smriprep). This will ensure we stay updated with the latest and greatest. I don't know why we reverted back to fixed releases only, but WDYT?

cc @effigies , @oesteban

@oesteban
Copy link
Member

oesteban commented Mar 4, 2021

Yes, pinning master breaks packaging tests as Pypi doesn't allow them anymore.

One option would be minimizing package tests if not a tag or rel/ branch.

On the other hand, cutting a pre-release is relatively cheap and probably worth forcing ourselves into doing it with certain frequency, as a way to catch dependency issues early.

This could be better replaced with daily automatic tests that replace dependency links with the bleeding edge release.

@mgxd
Copy link
Collaborator

mgxd commented Mar 4, 2021

Yes, pinning master breaks packaging tests as Pypi doesn't allow them anymore.
One option would be minimizing package tests if not a tag or rel/ branch.

I think this is the cleaner solution. Since master is our development branch, we should first concern ourselves with playing nice with our ecosystem over packaging, IMO. Running the pypi packaging tests on any tag should still catch release problems. Just to clarify, what I'm proposing is:

  • Pinning smriprep, niworkflows, and sdcflows to their development branches on our master. Our maint/U.X branches will still be bound by versioned releases.
  • Running the pypi packaging tests only on push tags.

On the other hand, cutting a pre-release is relatively cheap and probably worth forcing ourselves into doing it with certain frequency, as a way to catch dependency issues early.

If there is a way to automate this, sure. But less manual intervention sounds better to me 😄

@oesteban
Copy link
Member

oesteban commented Mar 4, 2021

  • Running the pypi packaging tests only on push tags.

Besides tags, I'd add the rel/* branches here. Maybe maint/* too.

If there is a way to automate this, sure. But less manual intervention sounds better to me 😄

What I meant is some sort of cron job (GH Actions and CircleCI offer them) that takes master, resolves the depency links to the latest (pre-)release of the dependency and runs the packaging tests (at the very least).

@mgxd
Copy link
Collaborator

mgxd commented Jul 12, 2021

I think this can be closed after #2409

@effigies effigies closed this Jul 12, 2021
@effigies
Copy link
Member Author

Agreed.

@effigies effigies deleted the fix/dependencies branch July 12, 2021 20: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.

Dependency errors stemming from scikit-image+numpy
4 participants