Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asyncio #1318

Merged
merged 16 commits into from Jul 21, 2020
Merged

Asyncio #1318

merged 16 commits into from Jul 21, 2020

Conversation

njwhite
Copy link
Contributor

@njwhite njwhite commented May 20, 2020

Add support for async UDFs (i.e. coroutines - see async_tests.xlsm) and hopefully fix array issues.

This should replace PR #1177 / fix #1164; the "disappearing arrays" happen because clear_contents is called successfully but the .formula_array = line silently fails due to the COM object being bound to the wrong thread (I think) - leaving you with empty cells. This PR introduces the ComRange subclass to handle marshalling the .impl objects correctly across threads.

Lays the groundwork for native Excel async functionality.

Add support for async UDFs and hopefully fix array issues. Most code
changes needed for COM support!
@fzumstein
Copy link
Member

This looks amazing! A few questions/notes:

@njwhite
Copy link
Contributor Author

njwhite commented May 21, 2020

@fzumstein Thanks - I fixed simple_coro and #1010 (and added its test case to async_tests.xlsm). Caveat: if you call formula_erased_2(1) formula_erased_2(2) formula_erased_2(1) Excel (2019) breaks when 2 returns as it seems to successfully set formula for 2's 20x250 part of 1's 20x300 array and then endlessly complains that you can't resize an array.

@fzumstein
Copy link
Member

awesome! Here are my thoughts:

  • simple_coro is still behaving synchronously for me, it doesn't return instantly with the #N/A waiting... something I'm overlooking?
  • It looks like formulas with legacy dynamic arrays (@xw.ret(expand='table')) are always called 2x, i.e. not taking the return value from the cache, but it may very well be fair to not support the legacy dynamic arrays anymore in the context of async functions as there are now the native dynamic arrays
  • UDF formula is erased on return #1010 seems to work great now when used with the native dynamic arrays

@njwhite
Copy link
Contributor Author

njwhite commented May 21, 2020

@fzumstein in this PR Excel still synchronously calls async def UDFs like def UDFs (they just let you write / run async code easily from UDFs). I'm planning to do another PR to get Excel native async working end-to-end with async def functions. Re: caching, I accidentally removed an else (!), should work again now.

@fzumstein
Copy link
Member

Cool, this fixed it. There one more issue though: When you open async_tests.xlsm (or do a Ctrl-Alt-F9), the threading functions never return (they don't get called in the first place) and are stuck on #N/A waiting.... They do work when they are called individually, but not when you recalculate the whole sheet.

@njwhite
Copy link
Contributor Author

njwhite commented May 22, 2020

I've pushed a fix for that but it's a bit messy. When you pass a COM object to another thread pythoncom calls into Excel to see what it can do with the object (GetTypeInfo()). If that call fails (like it does on a sheet recalculation) then the resulting Dispatch object is just broken and fails on every call. The PR now tests the object it's deserialized and retries if it's broken.

Alternative would be to pass the address / sheet / pid / &c. as parameters as a special case, but I think this helps document the odd behaviour for the next time someone hits COM bugs.

P.S. I accidentally reformatted udfs.py

@fzumstein
Copy link
Member

There's another issue I found: when you have many UDFs lined up, it does not manage to recalculate them all. See attached test case (should be added to the test actually). Change A1 to 2 and you won't get all 200 formulas recalculated with this error:

Traceback (most recent call last):
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 300, in delayed_resize_dynamic_array_formula
    c_y, c_x = caller.shape
  File "c:\users\felix\dev\xlwings\xlwings\main.py", line 1522, in shape
    return self.impl.shape
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 728, in shape
    return self.coords[3], self.coords[4]
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 699, in coords
    self.xl.Worksheet,
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\dynamic.py", line 527, in __getattr__
    raise AttributeError("%s.%s" % (self._username_, attr))
AttributeError: <unknown>.Worksheet
couldn't resize
Traceback (most recent call last):
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 300, in delayed_resize_dynamic_array_formula
    c_y, c_x = caller.shape
  File "c:\users\felix\dev\xlwings\xlwings\main.py", line 1522, in shape
    return self.impl.shape
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 728, in shape
    return self.coords[3], self.coords[4]
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 699, in coords
    self.xl.Worksheet,
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\dynamic.py", line 527, in __getattr__
    raise AttributeError("%s.%s" % (self._username_, attr))
AttributeError: <unknown>.Worksheet

See #881 for how it was fixed back then.

xlwingstest.zip

@njwhite
Copy link
Contributor Author

njwhite commented May 26, 2020

I've added your stress test to async_tests.xlsm and made it work! It turns our under heavy load (lots of COM calls) excel starts rejecting the calls to GetTypeInfo that pythoncom uses when deserializing objects (in a Dispatch constructor), which is why the while-true loop in impl is needed.

On top of that, the CDispatch objects were using late binding, so every (new) function call in Python was making another COM call to excel to get argument types, &c. - and these could fail. pythoncom just throws AttributeError and never retries if they do. Instead the PR uses EnsureDispatch in COMRange.impl (i.e. early, not late binding), so when you return from impl you know you can call any method on that object without the dispatch mechanism throwing AttributeErrors.

I've kept the retry mechanism from #881 - it just lives here instead.

I've added the async-properties module as a dependency to setup.py. I'm not sure how much xlwings cares about adding third-party dependencies, but if it's a problem I can change the properties to async def get_XXX methods instead.

...finally, for another time it'd be nice if pythoncom supported async COM calls, but that code lives in C and doing async stuff in C is ...hard.

@fzumstein
Copy link
Member

awesome! I'll give it some thorough testing again over the next days and let you know

@fzumstein
Copy link
Member

Few more observations/questions/suggestions:

  • When you recalculate the whole book via Ctrl-Alt-F9, the numpy_async regularly fails with this error:
async_thread failed
Traceback (most recent call last):
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 53, in async_thread
    base.formula_array = stashme
  File "c:\users\felix\dev\xlwings\xlwings\main.py", line 1221, in formula_array
    self.impl.formula_array = value
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 788, in formula_array
    self.xl.FormulaArray = value
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\__init__.py", line 482, in __setattr__
    self._oleobj_.Invoke(*(args + (value,) + defArgs))
pywintypes.com_error: (-2147418111, 'Call was rejected by callee.', None, None)
  • Yes, please, getting rid of async-property would be awesome as I'd like to keep the dependencies to what is available in Anaconda

  • Is there no way to keep late binding? The first time running the latest version I got this which I think is a very common issue:

File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\__init__.py", line 96, in Dispatch
    return __WrapDispatch(dispatch, userName, resultCLSID, typeinfo, clsctx=clsctx)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\__init__.py", line 37, in __WrapDispatch
    klass = gencache.GetClassForCLSID(resultCLSID)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\gencache.py", line 183, in GetClassForCLSID
    mod = GetModuleForCLSID(clsid)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\gencache.py", line 244, in GetModuleForCLSID
    makepy.GenerateChildFromTypeLibSpec(sub_mod, info)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\makepy.py", line 318, in GenerateChildFromTypeLibSpec
    gen.generate_child(child, dir_path_name)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\genpy.py", line 1040, in generate_child
    self.finish_writer(out_name, self.file, worked)
  File "C:\Users\felix\Miniconda3\lib\site-packages\win32com\client\genpy.py", line 801, in finish_writer
    os.rename(filename + ".temp", filename)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\felix\\AppData\\Local\\Temp\\gen_py\\3.7\\00020813-0000-0000-C000-000000000046x0x1x9\\Workbooks.py.temp' -> 'C:\\Users\\felix\\AppData\\Local\\Temp\\gen_py\\3.7\\00020813-0000-0000-C000-000000000046x0x1x9\\Workbooks.py'

@njwhite
Copy link
Contributor Author

njwhite commented May 31, 2020

@fzumstein ok addressed all of the above - although I'm pretty sure the first issue was always there as there wasn't a retry loop around AsyncThread.run! I've made it go back to late binding - _com now also retries all AttributeErrors as it assumes they've just come from a mangled Dispatch object.

It'd be nice if the Dispatch object had an am-i-broken property or a way to force re-initialisation as currently any work on the com_executor done before the first COM call is wasted if it turns out the Dispatch object is broken, but that's one for pywin32...

Also, it turns out Asynchronous support is not available for IDispatch or for interfaces that inherit IDispatch. so will have to keep the com_executor threadpool around for the foreseeable future :(

@fzumstein
Copy link
Member

great stuff! I'll let you know as soon as I am done with testing!

@fzumstein
Copy link
Member

Here's something new I found: when you open async_tests.xlsm and let the book finish calculating, then change A1 in sheet hello to 1, it fails a couple of times with this error:

couldn't resize
Traceback (most recent call last):
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 316, in delayed_resize_dynamic_array_formula
    await caller.clear_contents()
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 273, in clear_contents
    await self._com(lambda rng: rng.impl.clear_contents())
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 270, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  [Previous line repeated 13 more times]
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 270, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 264, in _com
    return await self._com(fn, *args)
  [Previous line repeated 28 more times]
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 258, in _com
    *args))
  File "C:\Users\felix\Miniconda3\lib\concurrent\futures\thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 273, in <lambda>
    await self._com(lambda rng: rng.impl.clear_contents())
  File "c:\users\felix\dev\xlwings\xlwings\_xlwindows.py", line 744, in clear_contents
    self.xl.ClearContents()
TypeError: 'bool' object is not callable

@njwhite
Copy link
Contributor Author

njwhite commented Jun 2, 2020

@fzumstein hm, I can't reproduce that on Excel 2019 / pywin32 227. Does it happen deterministically? If you've got time to help debug it, could you try setting the below somewhere in xlwings so it prints out lots of win32 debug info?

win32com.client.dynamic.debugging_attr = 1
win32com.client.dynamic.debugging= 1

My best guess is that due to the load (your stack trace is on retry 4 + 13 + 4 + 28 + 1!) a COM call failed that has made CDispatch think that ClearContents is an attribute.

Changing clear_contents in _xlwindows would help too:


def clear_contents(self):
    if self.xl is not None:
        fn = self.xl.ClearContents
        self.xl._print_details_()
        fn()

...otherwise I can put a call to CDispatch._FlagAsMethod in _xlwindows.Range.xl and see if that works.

[EDIT]
Also, I'm thinking of setting the number of threads in com_executor to something much less than the default (like 4) to reduce load on Excel, but that'd impact async functions under this PR (which use the same threadpool ...for now). What kind of thread-level parallelism do users expect from async_mode='threading'? Current behaviour spawns one thread per formula without limit.

@fzumstein
Copy link
Member

It's not deterministic in the sense that it always fails on a different amount of formulas. I am running this on a VM with restricted resources so might be coming from there. When I enable the two debugging statements, it usually doesn't fail as it's slowed down I guess. Only once it failed with these errors:

Calling GetIDsOfNames for property FormulaArray in Dispatch container <unknown>
Attribute ClearContents not in cache
Getting property Id 0x71 from OLE object
__setattr__ property FormulaArray (id=0x24a) in Dispatch container <unknown>
SetAttr called for <unknown>.FormulaArray='=hello(A1)' on DispatchContainer

@njwhite
Copy link
Contributor Author

njwhite commented Jun 3, 2020

@fzumstein ah got it - I was adding a random backoff to stop DoSing Excel when I started to repro it. It turns out that Excel does use early binding; it uses the CLSID to look up the pre-compiled object in a map (vs. what I was doing - compiling a new object every call). I've made COMRange stash the CLSID of the Range object and pass it to the Dispatch constructor in impl - pywin32 then uses this to look up the right compiled class and return it.

@fzumstein
Copy link
Member

I think this gets very close now! As usual, i have some comments ;)

  • It currently doesn't solve UDF with expand=table disappears on recalculation #1164, but as I was stating in the issue, I don't think that xlwings should allow expand='table' in the input args so I don't think it's an issue. Just wanted to mention it as you state in the very first comment that this solves it.
  • This implementation is notably slower for cases like the hello tab on the async_tests.xlsm: When you switch A1 between 1 and 2, it takes currently around 26s whereas on master it takes around 17s but I guess better slower instead of deleting formulas.

@njwhite
Copy link
Contributor Author

njwhite commented Jun 4, 2020

@fzumstein oh right, it didn't fix #1164 but now it does! Your comments there were right - it's a race condition as when the array is cleared it triggers a recalcuation of TableExpander, which finds an 1x1 array (as everything's blank). FWIW the example didn't disappear for me - it'd get stuck in and endless loop flicking between 2x3 and 1x1 arrays. The fix is to cheat when expanding tables and use HasArray - and if we're in an array then just expand to fill it rather than guess based on _empty cells.

I'd also expect it to be slower in every case due to the fact it's now retrying - master tries each calcuation once and eats the array if it fails (due to a COM error as Excel is busy), but the PR tries each calculation repeatedly until it's set successfully. I can't really think of a like-for-like comparison unless you comment out the PR's retry logic (in _com) as you have no control over whether Excel rejects a COM call.

xlwings/_xlmac.py Outdated Show resolved Hide resolved
@fzumstein
Copy link
Member

impressive!! I'll give this a few more days of testing and let you know if I find anything else or have more questions :)

@fzumstein
Copy link
Member

Ok one more thing: we should maintain compatibility with Python 3.6, currently I get:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "c:\users\felix\dev\xlwings\xlwings\__init__.py", line 37, in <module>
    from .udfs import xlfunc as func, xlsub as sub, xlret as ret, xlarg as arg, get_udf_module, import_udfs
  File "c:\users\felix\dev\xlwings\xlwings\udfs.py", line 30, in <module>
    initializer=pythoncom.CoInitialize)
TypeError: __init__() got an unexpected keyword argument 'initializer'

@fzumstein
Copy link
Member

@njwhite I see that asyncio changed quite a bit between 3.6 and 3.7 - do you have an idea how painful it is to make it compatible with 3.6?

@njwhite
Copy link
Contributor Author

njwhite commented Jun 15, 2020

@fzumstein I've pushed changes for 3.6 compatibility. It works, but:

  1. The code is messy!
  2. For some reason Range.impl.api.CLSID doesn't exist in my 3.6 / pywin32 227 environment. The PR still works - it's just using dynamic binding rather than the explicitly precompiled objects so may have some of the AttributeError race condition bugs above (not that I saw them in my testing). This could be because my Python environment is a bit broken though; I just did conda install python=3.6 and reinstalled numpy & pandas to downgrade so it might still have some 3.7 bits lying around. Deleting %TEMPDIR%/gen_py didn't help.

I appreciate the time you've put into testing this - I'm definitely still working on getting this over the line!

@fzumstein
Copy link
Member

@njwhite awesome! I'll give it another round of testing and let you know. I think messy code to some extent part of writing a library - even with 2.7 out of the picture ;)

@fzumstein
Copy link
Member

@njwhite unfortunately, this indeed reintroduced the TypeError: 'bool' object is not callable as described above...

@fzumstein
Copy link
Member

fzumstein commented Jun 23, 2020

I also got the Range.impl.api.CLSID when experimenting, but could fix it by running from win32com.client import makepy;makepy.main() after deleting %TEMP%/gen_py.

@fzumstein
Copy link
Member

@njwhite can you please have a look at my commit? let me know what you think. This only makes TypeError: 'bool' object is not callable appear in the edge case on py36.

@njwhite
Copy link
Contributor Author

njwhite commented Jun 30, 2020

@fzumstein I've tried your commit on 3.6 after clearing & rebuilding gen_py and I don't get errors any more! FWIW the errors on 3.6 aren't regressions in xlwings; 3.6 users just get partial fixes for disappearing arrays & the other bugs (vs. fixed in every case for 3.7+ users) - so I think this PR is a net positive even if we don't work out all the 3.6 issues...

@fzumstein
Copy link
Member

@njwhite sure I agree on Python 3.6. It's only going to be around for another 1.5 years anyways. I'll try to have this branch tested in the field before I merge, but other than that I think it's ready now?

@njwhite
Copy link
Contributor Author

njwhite commented Jun 30, 2020

@fzumstein great! let me know how it goes -

@njwhite
Copy link
Contributor Author

njwhite commented Jul 21, 2020

@fzumstein excellent!

@fzumstein
Copy link
Member

@njwhite sorry it took so long! I'll try to release to pip package tomorrow and the conda package a few days later.

@fzumstein
Copy link
Member

💥 this has now been released on PyPI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UDF with expand=table disappears on recalculation
2 participants