rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

Add a new python comment annotation that adds targets to `deps`

Open dougthor42 opened this issue 1 year ago • 2 comments

🚀 feature request

Relevant Rules

  • gazelle

Description

Some Python modules may have "hidden" imports. One prime example of this is pyvisa when using the pyvisa-py backend. A basic Python file that uses such looks like:

# foo.py
import pyvisa

if __name__ == "__main__":
    rm = pyvisa.ResourceManager('@py')
    print(rm.list_resources())  # ('USB0::0x1AB1::0x0588::DS1K00005888::INSTR')

Note that import pyvisa_py is not present in the file.

When Gazelle runs on this file, it generates:

py_binary(
    name = "foo",
    srcs = ["foo.py"],
    deps = ["@pypi//pyvisa"],
)

However, this fails to run with ModuleNotFoundError: No module named 'pyvisa_py'.

Describe the solution you'd like

Add a new Python annotation # gazelle:include_dep that allows users to add arbitrary targets to generated deps:

# foo.py
import pyvisa

# gazelle:include_dep @pypi//pyvisa_py

if __name__ == "__main__":
    rm = pyvisa.ResourceManager('@py')
    print(rm.list_resources())  # ('USB0::0x1AB1::0x0588::DS1K00005888::INSTR')
py_binary(
    name = "foo",
    srcs = ["foo.py"],
    deps = [
        "@pypi//pyvisa",
        "@pypi//pyvisa_py",
    ],
)

A corollary would be to also add a # gazelle:include_data annotation.

Describe alternatives you've considered

The current workaround is to import the module and then just not use it:

# foo.py
import pyvisa
import pyvisa_py

del pyvisa_py  # `del` it to satisfy linters.

if __name__ == "__main__":
    rm = pyvisa.ResourceManager('@py')
    print(rm.list_resources())  # ('USB0::0x1AB1::0x0588::DS1K00005888::INSTR')

However, this doesn't allow users to add dependencies on Python targets that happen to be (a) outside of the python_root and (b) auto-run during some event[^1].

[^1]: One prime example is pytest's conftest.py. Gazelle already has some code that automatically adds this, but Gazelle won't add it if conftest.py is outside of of the python_root dir. Perhaps I should make a separate Issue for such...

dougthor42 avatar Apr 23 '24 17:04 dougthor42

What about using something like

import pyvisa_py as _

Or modifying deps and adding keep directivev

These are two other options that allow us to handle this in a more idiomatic way.

In this particular case you could also use a requirement group that always bundles the two packages together at the pip.parse level, that way you don't need to annotate your python sources at all.

Given that there are three ways to solve this already, do we need a fourth one?

aignas avatar Apr 30 '24 14:04 aignas

import foo as _

Yes, this is a one-liner that accomplishes the same as import foo; del foo.

The issue with this method is that you can't add a dep that's outside of python_root. The other issue is that the import might be computationally expensive (hopefully not!) and users won't want always import it.

# keep

Well I wasn't aware about that until today, haha. Thanks!

This is better than import foo as _ because it allows deps that are outside of python_root and if we don't want to add the new annotation I'd be happy with using it.

However, it doesn't address the desire of wanting gazelle to add the dep. Admittedly that's a "nice to have" low-priority request. The reason I wanted gazelle to add the dep is so that non-SWE python users (think research scientists, hardware engineers, or even finance folk) don't have to interact with BUILD.bazel files at all.

Grouping at pip.parse level

This could work in some cases, where python package foo always "secretly" imports package bar. However, in cases where imports are dynamic (like pyvisa_py - it will only be imported if the arg to pyvisa.ResourceManager() is "@py") - then you're adding unused and possibly large files to the build path.

dougthor42 avatar Apr 30 '24 17:04 dougthor42