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

require doesn't work in some contexts, possibly due to bad function wrapper equivalence #302

Closed
wesgarland opened this issue Mar 18, 2024 · 11 comments · Fixed by wesgarland/python-launch-job#1
Assignees
Milestone

Comments

@wesgarland
Copy link
Collaborator

wesgarland commented Mar 18, 2024

Issue type

Bug

How did you install PythonMonkey?

None

OS platform and distribution

No response

Python version (python --version)

No response

PythonMonkey version (pip show pythonmonkey)

No response

Bug Description

There are two demo programs in my python-job-launch github repo; ping.py and ping-post-init.py

The are nearly identical, the difference is

39c39,40
< dcp_client['init'](go)
---
> dcp_client['init']()
> go()
[~/git2/python-launch-job] dev-wes:wes# 

The first one works and the second one doesn't. The reason it doesn't work is

Traceback (most recent call last):
  File "/home/wes/git2/python-launch-job/./ping-post-init.py", line 39, in <module>
    dcp_client['init']()
  File "/home/wes/git2/python-launch-job/node_modules/dcp-client/index.py", line 110, in init
    asyncio.run(load_dcp_client(*args, **kwargs))
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/wes/git2/python-launch-job/node_modules/dcp-client/index.py", line 51, in load_dcp_client
    dcp_client_modules = pm.eval("""'use strict';(
pythonmonkey.SpiderMonkeyError: Error in file /home/wes/git/pythonmonkey/python/pminit/pythonmonkey/node_modules/ctx-module/ctx-module.js, on line 238, column 21:
Error: module not found -- require('undefined/fs-basic') from undefined

The undefined happens because require can't figure out the relative path of dcp-client during init(). And it can't find it because somehow it's a Python function wrapper instead of a JS function, so the properties don't work.

Note that this needs the last dcp-client branches on the various repos to work.

Standalone code to reproduce the issue

No response

Relevant log output or backtrace

No response

Additional info if applicable

No response

What branch of PythonMonkey were you developing on? (If applicable)

No response

@karenbatch1997 karenbatch1997 added this to the v1.0.0 milestone Mar 20, 2024
@philippedistributive philippedistributive self-assigned this Mar 21, 2024
@philippedistributive
Copy link
Collaborator

@caleb-distributive
Copy link
Collaborator

caleb-distributive commented Apr 3, 2024

Wes is curious how a dcp-client PR fixed this, how is that possible?

@wesgarland
Copy link
Collaborator Author

There is no way AFAICT that a dcp-client fix could fix pythonmonkey function wrappers

The fix also requires a unit test demonstrating that a using the python require from JS has the correct properties and that relative require works

@wiwichips
Copy link
Collaborator

wiwichips commented Apr 3, 2024

@wes
@caleb-distributive

Refer to this comment on the MR

I'll take full responsibility for the mistake if my explanation isn't valid since I approved the MR.

@philippedistributive
Copy link
Collaborator

philippedistributive commented Apr 3, 2024

@wesgarland we fixed the issue reported above so we closed this ticket and discarded the seemingly incorrect hypothesis around

"The undefined happens because require can't figure out the relative path of dcp-client during init(). And it can't find it because somehow it's a Python function wrapper instead of a JS function, so the properties don't work."

as a "shot in the dark" since it was not a required consideration to fix the reported issue.

If there is another issue, we need a genuine reproducer even if we have it in this same ticket, which is not the obvious course of action. Perhaps we should have a new ticket to cleanly separate matters?

@wesgarland
Copy link
Collaborator Author

wesgarland commented Apr 3, 2024

So what's happened here is we have changed the library that reproduced the bug

I guess I will have to write another example to show that you can't use a require created from python in javascript

But I don't have time to do that today.

Note that dcp-client has absolutely nothing whatsoever to do with module resolution, ergo changing dcp-client changes the test but does not fix the bug.

@philippedistributive
Copy link
Collaborator

philippedistributive commented Apr 3, 2024

@wesgarland I don't think anyone ever claimed dcp-client had to do with module resolution.

We will need to bridge the gap between intent and perceived intent.

This discussion is getting very much twisted, it seems we are retro-arguing to explain an initial state of facts which no longer exists due to how it was conveyed and then perceived :-)

Indeed there are potentially two bugs here, and the one most manifest is the one detailed with the provided code sample and console log. Peripheral comments, when deemed irrelevant to the code example, if still relevant to another issue, belong in a new/separate ticket. Continuing to try to hammer in the concept that there is only one bug or that the bug is not the one that was fixed is a clear dead end at this point.

We focused on the manifest issue thinking the supplemental discussion/thoughts to be non-essential since they were not presented as such, indeed it seemed to be speculation as to the cause of the one single issue, and once that single issue was solved (there sure was an issue to fix there in dcp-client), it seemed to confirm the hunch that the other elements were purely speculative, indeed a "shot in the dark".

There was no evidence that the ticket reporter was aware of the existence of two separate issues, hence fixing the manifest issue could naturally be construed as fixing all reported issues

We found a solution to the reported issue that was surprising and unexpected, and did not go along the hypothesised diagnostic, I understand that. Yet it does not diminish the value of the solution to the existing problem. And it is natural that the hypothesised diagnostic be dismissed if it appears to be irrelevant to the solution, and that it needs to be re-introduced if thought to still be worth investigating. This also entails that the hypothesised diagnostic was so far in nature to the actual solution that it was not deemed to be worth further consideration.

Hence it required to be risen from the dead by the ones who believed in it 🙂

@philippedistributive
Copy link
Collaborator

@wesgarland also if you say we changed the library which reproduced the bug, you mean here index.py, you could simply say that the reproducer is to backtrack that change

@philippedistributive
Copy link
Collaborator

@wesgarland was the remaining issue here reformulated as #331 ?

@wesgarland
Copy link
Collaborator Author

331 doesn't have a title which is really on topic, it is one example of the larger problem, which is that function property lookup doesn't work. Caleb mentioned that halfway through the discussion.

@caleb-distributive
Copy link
Collaborator

Checked just now and #302 is resolved afaik. A JS function passed to python and back to JS is still the exact same object, with all the same properties etc.

Python 3.11.8 (tags/v3.11.8:db85d51, Feb  6 2024, 22:03:32) [MSC v.1937 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import pythonmonkey as pm
>>> pm.eval("function myJSFunction() { console.log('hello there'); }")
>>> pm.eval("myJSFunction.a = 42")
42.0
>>> wrappedJSFunc = pm.eval("myJSFunction") 
>>> pm.eval("(func) => func === myJSFunction")(wrappedJSFunc)
True
>>> pm.eval("(func) => console.log(func.a)")(wrappedJSFunc)
42    

However, #331 is a different issue. That issue demonstrates that properties on JS functions (both ownProperties and properties on the prototype chain) are inaccessible from python:

>>> wrappedJSFunc.a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'pythonmonkey.JSFunctionProxy' object has no attribute 'a'
>>> wrappedJSFunc.toString()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  AttributeError: 'pythonmonkey.JSFunctionProxy' object has no attribute 'toString'  

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

Successfully merging a pull request may close this issue.

5 participants