This repository was archived by the owner on Nov 5, 2019. It is now read-only.
Detect when numpy function calls don't modify parameters #1
Description
As shown in microsoft/gather#23, gather will detect methods like np.sin as modifying the np object, thus including these calls in slices where they don't belong.
As an example, consider the following two cells. Try to gather the output of the second cell. You'll find nbgather will gather the line np.sin(10), even though it's not necessary in the slice.
# Cell 1
import numpy as np
np.sin(10)# Cell 2
print(np.arange(0, 10))
Activity
andrewhead commentedon Jul 19, 2019
One fix that I'd be up for implementing is to modify the default slicing settings to include a list of common
npmethods that don't modify their parameters, here:https://github.com/microsoft/gather/blob/0ad10fd2c4ecd01dc4857c8e779b6c01ded2e00f/schema/plugin.json#L48-L74
joyceerhl commentedon Jul 19, 2019
This also applies to
pandasDataFrame methods likedf.tail(), and plotting methods from common libraries likebokeh.plotting.plot()andmatplotlib.pyplot.plot(). But I could also see how this might get unwieldy/too tightly coupled with the libraries in question and/or create problems down the line if the implementation of those functions changes.andrewhead commentedon Jul 21, 2019
Agreed, pandas DataFrame methods and matplotlib's plotting functions are part of this issue too.
I appreciate your point about tight coupling / getting unwieldy. My feeling is that these libraries are so widely-used that I can't imagine the side effects of their APIs on the arguments changing anytime soon. For now, I'm okay with coupling, and dealing with re-implementation of those APIs if it happens.
joyceerhl commentedon Jul 22, 2019
A related (possibly separate?) issue is that the slice configuration assumes users will import library functions and objects by standard names (e.g.
import numpy as np, declaring a pandas DataFrame aspd).For example, if I execute the following code and gather the output of the
dataframe.tail()function call, the gathered code would also includedataframe.head(), even though the default slice configuration rules specify thatdf.head()does not modify thedfobject:greazer commentedon Jul 22, 2019
+1 to what Joyce says above. Wouldn't we want to specify API's that don't modify their associated objects by type rather than by name?
andrewhead commentedon Jul 23, 2019
Jim Griesmer (@greazer) Joyce Er (@joyceerhl) Yes, totally! It would be more flexible, and more accurate to specify what APIs modify their objects using an object's type than its name.
The name-based approach is used here just because it was an easy-to-implement approximation and straightforward to extend with new rules. That said, it has its clear pitfalls.
I've thought a bit about the right way to incorporate type inference into this plugin. For example:
I'm interested to hear if anything comes to mind for either of you for alternate implementations of type inference, and also if there are restrictions in your use cases that would rule out one or the other. I'm open to doing type-based resolution of object modification rules, just would want to do it right.
andrewhead commentedon Jul 23, 2019
Joyce Er (@joyceerhl) While we discuss more stable, potentially long-term type inference approaches to resolving function names, would it be useful in the use cases you're addressing if the default rules for
npandpdwere updated using the current name-matching system? It's a quick fix I could easily make now, given that I think the conventions ofnpandpdare standard to the extent I can't imagine users redefining / overriding the functions on objects of those names.greazer commentedon Jul 23, 2019
Andrew Head (@andrewhead), in our case, we are using gather within the VSCode python extension. Therefore, we already have a language service that can provide us type information. The protocol for the language service is public domain and documented here standard language service protocol. It would seem to me that the gather library could be made to support this protocol in order to retrieve type info. Supporting this protocol would provide an answer for us as well as other tools (like Jupyter) to be able to supply the same info.
Perhaps more problematic though is that I think the gather mechanisms may rely on absolutely knowing the types of objects being executed. Because of Python's duck typing, using static source analysis (ala the language service) may not be entirely sufficient. However, it would still likely be way more accurate and general than the current name-based filtering.
andrewhead commentedon Jul 24, 2019
Jim Griesmer (@greazer) I like your suggestion to look into the standard language service protocol. For sure, I imagine it would being easier to use a portable, stable type checker that's been tested in other tools like VSCode than the other options I mentioned.
I have a couple newbie questions about standard language service protocols. Does a user need network availability to query the service? Or do they usually get accessed from some local copy of the service that gets downloaded when someone installs VSCode Python dev tools?
If there's some way to package up one of the existing service implementations for Python and make it a dependency for the package, I can see it as a no-brainer to make analysis more robust.
andrewhead commentedon Jul 24, 2019
Great point. It's a good reason to incorporate type information too.
For a bit of context, gather's currently static is to keep installation simple (no need to install an instrumented Python kernel for dynamic slicing). That said, we want to make the slicer do what people expect it to do, and make the code useful for others. Maybe dynamic is the right call long-term, if implementation work was properly scoped and there was an easy way to distribute the algorithm.
For notebooks, we figured the occasional slice gaffe with easy back-out (non-destructive gather operations) would be useful, and we'd work to minimize those gaffes. That said, if user expectations / hopes differ from this for VSCode users, it would be handy to discuss because better slicer for one group of users would probably be better for all.
rdeline commentedon Aug 28, 2019
The code now has the start of a solution to the issue being discussed. There's a new spec language (set of json files) for describing what we need to know about an API in order to do slicing on code that calls that API. Basically we need to know what functions/methods have side-effects on their parameters, including the self parameter. Because Python is weakly typed, we also need to know when functions/methods return types that have specs. The analysis does rudimentary type inference, in order to know whether a called method has a side-effect.
With this new spec system, we can do our old demo examples, without the need for any name-matching hacks. So far, the specs have had minimal investment, so there could easily be mistakes. Also, down the road, I intend to create a tool to automatically produce the specs from Python source code. For now, writing them by hand isn't too bad, especially since a handful of libraries account for a huge fraction of the total API calls.
I'm going to close to issue, since the original reported bug is now fixed.