DueCredit not imported

[jbteves]

When using a miniconda3 environment or pip install, the following import error occurs:

Failed to import duecredit due to No module named 'duecredit'

Unless we are avoiding acknowledging the devs and their hard work, we should probably fix that :-)

[tsalo]

Currently, duecredit is an optional dependency, so it should raise a warning when it isn't available. While I wish that it could be called from within tedana, unfortunately we need to call any tedana-calling script with duecredit specifically in order for it to run (e.g., python -m duecredit run_tedana.py). I don't think many people actually do that, even though we have information about how to in the documentation, so I think it makes the most sense as an optional dependency.

We did notice a little while ago that the warning that was being raised wasn't very informative (see here), so we've since changed it to say the following when duecredit is not available:

Module duecredit not successfully imported due to "No module named 'duecredit'". Package functionality unaffected.

I guess that that happened after the most recent release, so it should show up starting in the next one.

[jbteves]

I think it's just confusing for the user to receive the warning in a context where they're not looking for a citation, it makes it seem as if the setup has gone wrong. Would it be possible to not raise a warning at all under normal operations, but then raise an error if they're trying to get the duecredit message and it can't find the library?

[tsalo]

I think it's possible and a good idea, though that's something that should probably happen upstream in duecredit.

[jbteves]

Taking a look, it seems like that may be a complicated approach. This may be inelegant, but can we just catch that warning and then ignore it? I see that here we catch the import exception, could we add a check for the name on the bottom of the stack, and discard the message entirely if it's not duecredit? Additionally, I think it may be ignored in documentation because it's below the citations. Docs state you can get citations "additionally" by running that, but most people won't run it if they think that the relevant ones are listed already and can just be pasted into the paper. Just a thought.

[emdupre]

I'm against hiding the warning, personally. I think the new message that @tsalo introduced should alleviate a lot of this confusion (and this conversation is a good reminder that we should think about cutting a release, soon !).

Regarding the placement in the documentation: I completely hear your point, but I'm not sure where would be better to put it. Do you have a suggestion ?

[jbteves]

RE: warning May I ask why you're against hiding the warning? I tend to believe that warnings are scary for new users, so I try to minimize them unless I think it needs to be brought to their attention. Thus, I figured that if it's not hindering the program in that instant, we should not raise one. I assume there's some reason that it's put there, though, so I thought it might be good to discuss on this issue in case this comes up again and all of the rationales are in one place.

RE: doc placement I'm afraid I don't have a good solution-- it's a classic question, right, how do you get users to read the full docs? (Obviously I'm guilty myself). The only other way I can think of is to add a reminder at runtime to please cite, but people may find that frustrating/annoying.

[jbteves]

@emdupre unfortunately with another user I've interacted with was still concerned that something is wrong with the setup, even with the new message. Nobody has filed or commented on a bug report except me, so I'm not sure how widespread this reading of the warning is.

[jbteves]

It's been a while since we discussed this and nobody else seems bothered by the warning, so I think we can close this.

[emdupre]

Sorry for never following up -- I was just concerned about hiding the warning for users who do want to use duecredit ! I think closing for now is a good call, though, and we can always re-open with new reports :smile:

[jbteves]

@handwerkerd Here's the relevant issue thread.

[jbteves]

I would like to reopen this issue as a few more NIH users have reported concern over duecredit. It's a small dependency relative to our other dependencies and I feel that if we want to use it we should just require it.

[tsalo]

What is the nature of their concerns? The warning message is pretty clear IMHO.

[jbteves]

They just ask me if it's supposed to be there. At this point I value a few megabytes of data and a line of requirements to be less important than having users ask me if it's okay, even though it says it's okay. I will concede that, to their credit, usually "not successfully"/"Failed to" juxtaposed with "functionality unaffected" is a little confusing.

[tsalo]

Fair enough. I agree that it's a very minor requirement, and it won't cost us anything to add it.

[tsalo]

#875 addressed this, so I'll close it now.

[mateuszpawlik]

When I install Tedana with pipx in a Docker image, I still get the duecredit message:

root@6ccbea0e2c6a:/# tedana --version
Failed to import duecredit due to No module named 'duecredit'
tedana v23.0.1

UPDATE pipx inject does the trick. Here's my entire Dockerfile:

FROM debian:stable
  
# Install packages
RUN apt-get update

# Install pipx for installing Python applications
#
# https://hub.docker.com/r/gesellix/awsume/dockerfile
#
RUN apt-get -y install pipx
ENV PIPX_HOME=/opt/pipx
ENV PIPX_BIN_DIR=/opt/pipx/bin
ENV PATH=/opt/pipx/bin:$PATH

# Install Tedana
RUN pipx install tedana==23.0.1
RUN pipx inject tedana duecredit