add nnpdf dependency to 'test' group

[RoyStegeman]

To avoid pytest from failing we need nnpdf to be downloaded as part of the pypi upload workflow: https://github.com/NNPDF/workflows/blob/v2/.github/workflows/python-poetry-pypi.yml

Changing that workflow makes it less general, while adding nnpdf to 'test' dependencies doesn't look very nice, but making non-optional is perhaps also overkill (or we can skip the failing test_utils.py test of the nnpdf_data package). What is the preferred solution?

[alecandido]

@RoyStegeman you could have it both optional (for the package, such that users can specify the extra) and as a test dependency, because it's true that to run (some) tests you'll need it (DD:).

Do you see any problem with that?

[RoyStegeman]

What is the benefit of having it twice? It seems version restrictions are still resolved even if the dependency is in a group: https://python-poetry.org/docs/managing-dependencies

so I think this means that if we do poetry add nnpdf it's the same whether we put it twice or not.

[alecandido]

They are fundamentally two different things.

Dependencies outside the groups are those entering the package, so they will be resolved also by the users installing from the wheel (e.g. from PyPI) and not from source. For them, pyproject.toml will not be available, nor poetry.lock, but only the *.dist-info/METADATA file generated by the build process. The extra is needed to enter that file, and be distributed to the user.

Instead, the group is a feature required to manage the environment. You decide for what you need that, but my personal advice would be that if you have a group named "test(s)" it should contain all the dependencies required to run the tests. Thus, it is worth to repeat the optional dependencies with the implied meaning "if I'm running the tests, this is not optional any longer".

This is the rationale, and to me ugly it's not ugly to have either, and advisable to have both. But of course you could handle it also in different ways :)

[scarlehoff]

I agree, if it is an optional in one case and necessary in the other so be it, it needs to appear in both.

[felixhekhorn]

Mmm I don't like this change, because it makes nnpdf mandatory

counter proposal: we add a (pytest) marker and if necessary we can also add a shortcut for selecting (similar to eko)

[scarlehoff]

Mmm I don't like this change, because it makes nnpdf mandatory

Only for tests though. pypi should not see nnpdf right? (only via pip install pineko[nnpdf])

[felixhekhorn]

Mmm I don't like this change, because it makes nnpdf mandatory

Only for tests though. pypi should not see nnpdf right? (only via pip install pineko[nnpdf])

yes - let me be clearer: "it makes nnpdf mandatory for me, which I try to fight" 🙃 (and actually this fight could even be wise for both nnpdf and pineko in terms of dependency management

[scarlehoff]

But either it is mandatory for the tests or it doesn't get tested...

[alecandido]

Personally, I would have liked that Pineko would have depended on nnpdf_data alone, rather than nnpdf, like the following:

flowchart RL
    nnpdf --> nnpdf_data
    pineko --> nnpdf_data

Loading

instead of

flowchart RL
    pineko --> nnpdf --> nnpdf_data

Loading

since nnpdf_data is truly the only thing required by Pineko (unless I'm still missing something...).

However, for as long as things stay as it is, I guess it shouldn't be a big deal, if only TensorFlow was left as an optional dependency in nnpdf (instead of entering all the isolated environments or forcing everyone to use --system-site-packages, which is renown as bad practice for development).

[scarlehoff]

It only depends on nnpdf_data, technically... but someone needs to create the nnpdf_data package :__ (and I don't want to distract human resources from the data-implementation tasks in which everyone should be concentrated now! But I'll make it a priority afterwards!)

[felixhekhorn]

I reread the head of this PR more carefully and the problem @RoyStegeman tries to solve is the PyPI workflow. However, the workflow allows to specify extras which are installed along the way:
https://github.com/NNPDF/workflows/blob/e681e4d636ae8000bd806e8124119c180dd7c6b7/.github/workflows/python-poetry-pypi.yml#L16
and in the workflow this is not active yet:
https://github.com/NNPDF/pineko/blob/main/.github/workflows/pypi.yml
so why don't we just add it there?

[RoyStegeman]

Ah you're right @felixhekhorn! I hadn't appreciated that option. That is indeed the kind of solution I was looking for to solve the issue (and a separate one from the packaging of nndpf_data).

Or, if we go with @alecandido's rationale, nnpdf should be part of the test group of course

[RoyStegeman]

In the end I agree with @alecandido's reasoning - if we want to do test we need download also nnpdf so this should be part of the test group.

If you are happy with the current solution, can someone approve this PR?