Hide deprecated callables from code completion suggestions

[binste]

Closes #2574 (code builds on suggestion of @joelostblom). Concern mentioned [here] is addressed by placing the statements at the end of __init__.py so that the module is fully imported.

In JupyterLab (and notebook) this works for me:

The deprecated functions themselves are still accessible

Unfortunately, in VS Code the deprecated functions still show up. I didn't find anything online on why Pylance does not respect __dir__. Maybe it also has a cache somewhere which I did not manage to clear. But I think it's already great if this is resolved for Jupyter.

[mattijn]

Is this comment related why pylance doesn't work? microsoft/pylance-release#1277 (comment)

[binste]

Just tested this and it still does not work for me in VS Code. Seems like pylance does not subset suggestions based on __all__, see microsoft/pylance-release#3709.

But it's a good point, probably better anyway to declare __all__ with explicit values so that more tools can use it. I also added a test so we have an automated check to make sure that we keep __all__ up-to-date.

[binste]

Could you rerun the tests? I think this is a spurious error which already appeared in other PRs and not related to this one.

[mattijn]

I’m not exactly sure if this is what we need, so I leave it open so others can discuss about this PR. Regardless needed, I think it is better to include this as part of the tools/generate schema wrapper, otherwise you’ll need to modify it manually with a new schema.

[joelostblom]

I like this change. I don't understand the import logic well enough to assess if this is enough to to satisfy @jakevdp 's comment, but it sounds like you have thought about how to fix that.

[binste]

Jakes comment was related to a previous implementation which dynamically fetched the relevant attributes from the module while it was imported. This is no longer relevant in this implementation as __all__ contains hardcoded strings so that static code analysis tools can use it as well.

[binste]

tools/generate_schema_wrapper.py so far does not touch __init__.py but I could also try to add it somehow there. However, __all__ also needs to be changed if files change which are not autogenerated so it doesn't solve the manual maintenance completely. Although one could then at least rerun tools/generate_schema_wrapper.py to update it so that's something. Altair would need to be in an importable state at the end of tools/generate_schema_wrapper.py. I'm not familiar enough with running it for a new vega-lite version if this is already the case or if first some issues need to be fixed by hand before this is the case.

[mattijn]

Maybe @ChristopherDavisUCI can think with us a bit here, as he has become quite familiar with the generate_schema_wrapper.py. Chris, would you mind to have a thought on this topic and the comment from Stefan?

[ChristopherDavisUCI]

Thanks @mattijn and @binste, sure, I will try to take a look at this and #2819 in the next few days.

[ChristopherDavisUCI]

Hi @binste, how did you choose the current list for __all__ in this PR? I'm not sure how likely it is that I'll have anything helpful to add, but I'll try to think if I can see a way that might automate the process.

[binste]

Thanks @ChristopherDavisUCI for looking into it! I generated the list with the code I added to tests/test_toplevel.py, i.e.:

import altair as alt

[x for x in dir(alt) if not getattr(getattr(alt, x), "_deprecated", False)]

[ChristopherDavisUCI]

Thanks! I looked around some more but don't have any alternatives to suggest.

I feel like this should be a common issue (wanting to hide a definition from Pylance) but did not find much of anything in my Google searches. (I found lots of examples of people wanting to hide Pylance warning messages.)

Is there any issue that we want selection_single and selection_multi to be available in v4 but this __all__ is being defined at the highest level?

I asked ChatGPT how to hide these functions, and it said to precede the definitions with the comment # pylance: disable but as far as I can tell ChatGPT just made this up.

@mattijn I'll try to look at #2819 soon, but I don't think I'm going to have anything else to suggest for this PR.

[mattijn]

I tried quickly to create this __init__.py file from within the generate_schema_wrapper.py, see the code-diff changes here: master...mattijn:altair:generate-init-file, but it seems the __init__.py-file is still created using the installed altair from within my side-packages, and not the one defined in ROOT_DIR:

# Import Altair from head
ROOT_DIR = abspath(join(dirname(__file__), ".."))
sys.path.insert(0, ROOT_DIR)
import altair as alt  # noqa: E402

[mattijn]

If I run the script directly from cmd it finds my local altair package in the directory as defined in ROOT_DIR.

Now I can run first (using: master...mattijn:altair:generate-init-file):

python tools/generate_init_file.py --no-hide-deprecated

To initialise the basis __init__.py file, and then:

python tools/generate_init_file.py --hide-deprecated

This creates the __init__.py file including the populated __all__ directive.

But.. the methods/functions that are deprecated are not ignored.
For me the _deprecated attribute is not found.

For example the alt.selection_single() function is deprecated in v5, but it cannot find a _deprecated attribute:

import altair as alt
sel_single = getattr(alt, "selection_single")
hasattr(sel_single, '_deprecated')

False

But calling the function directly, will present the AltairDeprecationWarning

sel_single()

/Users/mattijnvanhoek/mattijn/altair/altair/utils/deprecation.py:65: AltairDeprecationWarning: 'selection_single' is deprecated.  Use 'selection_point'
  warnings.warn(message, AltairDeprecationWarning)
Parameter('param_1', SelectionParameter({
  name: 'param_1',
  select: PointSelectionConfig({
    type: 'point'
  })
}))

So I was wondering, how can this PR pass CI, while I cannot reproduce it locally. But now I think this test: https://github.com/altair-viz/altair/blob/760bd0fa03546ca61a575cac11204418a5590ae2/tests/test_toplevel.py#L4 is successful because it checks itself, basically

dir(alt) == getattr(alt, "__all__")

[binste]

• Removed dunder attributes (__...__) from __all__ as those are not relevant for the normal user. Same approach is taken by pandas
• You're right @mattijn! That test did not test much at all... I fixed it by referencing alt.__dict__.keys() which still has all attributes defined on the top-level
• _deprecated is also defined in this PR. As far as I can see, it's missing in your branch. I see the point that it would be great to automate this and your approach seems to already work nicely. As an alternative, we could also just replace the __all__ definition in the __init__ file instead of completely generating it from scratch. I'll dig a bit into your code to better understand it and see if updating the file is also a viable option.

Tests currently fail, I'll address this soon.

[mattijn]

As an alternative, we could also just replace the all definition in the init file instead of completely generating it from scratch.

I tried this firstly, but in my approach I had to reload the package altair during runtime of the generator, which gave me other problems. Open for alternatives though!

[binste]

I opened a PR #2872 to fix the imports in those scripts. After merging this, I think we can update the __init__ file at the end of generate_schema_wrapper without reloading. Curious to hear what you think of it. Maybe putting the functionality into a separate script similar to generate_api_docs called update_init_file. I already have some sample code which I can finish once the other PR is merged.

[mattijn]

I think I would favour a expected.sort() here, since it will sort the list in-place and it make the list comprehension a bit more readable instead of the sorted() on top of the list comprehension.

But after checking I have one concern and one question.

After this PR the following items will not appear in the code completion suggestion:

__name__
__doc__
__package__
__loader__
__spec__
__path__
__file__
__cached__
__builtins__
__version__
__all__
__dir__
selection_multi
selection_single
list_datasets
load_dataset
datum

All good. Except the last one, datum, as it is not deprecated.
Since:

import altair as alt
getattr(alt.datum, '_deprecated', False)

datum._deprecated

This alt.datum is a bit of weirdo, it is a function, but can also be used to reference attributes. After reading this line, I think we can make this work if we change _deprecated into __deprecated__ since;

import altair as alt
getattr(alt.datum, '__deprecated__', False)

False

Or modify the __getattr__ method of DatumType class into something else.

And the question that I had is the following, after sorting the list starts with methods that are starting with an upper-case and after this the small-case methods appear in the sorted list. Is this something that we like? I noticed it is the same for the pandas library.

[binste]

Addressed your comments, added datum by checking if getattr(alt.datum, '_deprecated', False) is True. Also added update_init_file.py which updates __all__ automatically. It is called from generate_schema_wrapper.py so that no extra step is needed when updating altair from vega-lite. If another new object is added to Altair which does not come from the schema files and should be included in __all__ (e.g. a change in the API), test_toplevel.py would fail and instructs the user to run tools/update_init_file.py.

The attributes which are excluded from __all__ are now:

[
    "__all__",
    "__builtins__",
    "__cached__",
    "__dir__",
    "__doc__",
    "__file__",
    "__loader__",
    "__name__",
    "__package__",
    "__path__",
    "__spec__",
    "__version__",
    "hashlib",
    "io",
    "itertools",
    "json",
    "jsonschema",
    "list_datasets",
    "load_dataset",
    "pd",
    "pkgutil",
    "selection_multi",
    "selection_single",
    "warnings",
]

Regarding the sort order of upper- and lowercase, I don't have a good understanding of what the implications are. Given that both upper- and lowercase objects are relevant for end users (alt.Chart, alt.condition, ...) maybe it doesn't matter too much? And as soon as you start typing something after alt such as alt.sel you get the specific suggestions anyway.

[mattijn]

Thanks @binste, I tried to run the update_init_file.py from within the generate_schema_wrapper and as stand-alone, and it is all stable now. Also when re-running it won't error out. Thanks again, this is nice!