databrickslabs / ucx

Automated migrations to Unity Catalog
Other
217 stars 75 forks source link

[BUG]: Linting raises `library-not-found` when importing an unresolvable library within `contextlib.supress(ImportError)` #1706

Closed JCZuurmond closed 2 weeks ago

JCZuurmond commented 3 months ago

Is there an existing issue for this?

Current Behavior

We raise an library not found problem with the following code snippet.

with contextlib.suppress(ImportError):  # Module not available when building docs
    # ensure the object constructor is known by polars
    # we set this once on import

    # This must be done before importing the Polars Rust bindings.
    import polars._cpu_check

    polars._cpu_check.check_cpu_flags()

    # we also set other function pointers needed
    # on the rust side. This function is highly
    # unsafe and should only be called once.
    from polars.polars import __register_startup_deps

    __register_startup_deps()

Similar to #1705

Expected Behavior

No problem should be raised. The library is not found, however, that is as expected

Steps To Reproduce

  1. Create test for above code snippet

Cloud

Azure

Operating System

macOS

Version

latest via Databricks CLI

Relevant log output

No response

ericvergnaud commented 3 months ago

This is confusing. What is the rationale for not raising a library-not-found problem when a library is not found ?

ericvergnaud commented 3 months ago

Oh you mean because of suppress(ImportError) ? My recommendation would be to provide a mechanism for manually dismissing noisy advices, rather than tackle such complex scenarios

JCZuurmond commented 3 months ago

An alternative to suppressing the message is fine to me. I like the separation of concern of first detecting the library-not-found and then suppressing it in certain cases. I would prefer an automated approach over manual, depending on the frequency it occurs

nfx commented 3 months ago

we need to see if the originating import node has try: ... except: ... blocks. or with contextlib.suppress(ImportError) would be fine as well.

ericvergnaud commented 2 weeks ago

Already fixed, closing.