-
-
Notifications
You must be signed in to change notification settings - Fork 420
Added vizdoom python interface file to aid IDE for hinting #620
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
Conversation
…ck errors but that's for others to fix)
…rapper) pass pre-commit
Hi @Trenza1ore, thank you very much for this contribution. This indeed seems to be helpful. Can I ask how you specifically generated the pyi file? It would be nice to have a nice way to update it in case of changes to docstrings/new methods. |
Hi @mwydmuch it should be me thanking you guys for maintaining this awesome repo and make RL with Doom so much easier. My under/postgrad thesis wouldn't have been nearly as cool without the amazing library of ViZDoom. For the creation of the pyi file, here are the steps I've taken:
I am looking for a way to automate the workflow (especially manual checking part), will share any tool that aided me here (although for future incremental update that might be less useful). Also I expect to need one more day to finish validating the |
I believe that's it, the error: vizdoom.vizdoom.DoomGame is inconsistent, metaclass differs
Stub: in file /Users/Sushi/Desktop/ViZDoom/.conda/lib/python3.11/site-packages/vizdoom/vizdoom.pyi:631
N/A
Runtime:
<class 'pybind11_builtins.pybind11_type'> Here's my script for validating correctness via import re
import os
import platform
import subprocess
os.system("cls" if platform.system() == "Windows" else "clear")
os.system("sh ./update.sh") # copies latest vizdoom.pyi to installed ViZDoom 1.2.4
filter_placeholder = re.compile(r"error: (vizdoom\..*) variable differs from runtime type (vizdoom\..*)"
r"\nStub:.*\ntypes.EllipsisType\nRuntime:\n.*[0-9]+>\n*")
filter_builtin_methods = re.compile(r"error: (vizdoom\..*__.*__) is not present in stub\nStub:."
r"*\nMISSING\nRuntime:\n.*\n*")
filter_pybind11_metaclass = re.compile(r"error: (vizdoom\..*) is inconsistent, metaclass differs\nStub:."
r"*\nN/A\nRuntime:\n.*\n*")
filter_missing_attribute = re.compile(r"error: (vizdoom\.(?:vizdoom\.)?(.*)) is not present in "
r"stub\nStub:.*\nMISSING\nRuntime:.*\n(.*)\n*")
extract_function_info = re.compile(r"error: (vizdoom\.(?:vizdoom\.)?(DoomGame)\.(.*)) is not present at "
r"runtime\nStub:.*\ndef (\((.*?)\)( -> .*?\.(.*))?)\nRuntime:\nMISSING\n*")
def filter_all_error_messages(error_messages: str) -> str:
# error_messages = filter_placeholder.sub("", error_messages)
# error_messages = filter_builtin_methods.sub("", error_messages)
error_messages = filter_pybind11_metaclass.sub("", error_messages)
# error_messages = filter_function_false_negatives(error_messages)
# error_messages = filter_missing_enum_val_false_negatives(error_messages)
return error_messages.strip()
def filter_function_false_negatives(error_message: str) -> str:
with open("src/lib_python/vizdoom.pyi", "r", encoding="utf-8") as f:
stub_code = f.read().splitlines()
for match in extract_function_info.finditer(error_message):
class_name = match.group(2)
function_name = match.group(3)
function_signature = match.group(4).replace("vizdoom.", "").replace(class_name, "").strip().replace("self: ", "self").replace("builtins.", "")
function_return = match.group(4)
if any(function_signature in line for line in stub_code):
error_message = error_message.replace(match.group(0), "")
return error_message
def filter_missing_enum_val_false_negatives(error_message: str) -> str:
with open("src/lib_python/vizdoom.pyi", "r", encoding="utf-8") as f:
stub_code = f.read().splitlines()
for match in extract_function_info.finditer(error_message):
attribute = match.group(2)
if any(attribute in line for line in stub_code):
error_message = error_message.replace(match.group(0), "")
return error_message
stub_test_result = subprocess.run(["stubtest", "vizdoom"], capture_output=True, text=True)
std_out = filter_all_error_messages(stub_test_result.stdout)
# Save output to file for later analysis
with open("stubtest_output.txt", "w") as f:
f.write("=== STUBTEST OUTPUT ===\n")
f.write(f"Return code: {stub_test_result.returncode}\n\n")
f.write("=== STDOUT ===\n")
f.write(std_out)
f.write("\n=== STDERR ===\n")
f.write(stub_test_result.stderr) |
Also I noticed that there seems to be an undocumented SignalException, not sure what it's for. |
Additionally add a enum_typing example to demonstrate expected duck-typing behaviour of ViZDoom Enums (which should be represented as subclass of Python's
Also in retrospect the workflow could have been much better and highly automated.
|
Thank you for describing the process! It seems that building mypy stubgen fails to recognize some signatures, like docstrings from the pybind11 extension. Adding them using LLM is not reliable and cumbersome to automate. There is basically this package https://github.com/sizmailov/pybind11-stubgen to generate stubs for pybind11 modules. By running |
This one is no longer used, and if something like this is still in the codebase, then it's a leftover from the old signal handling system that should be removed. |
I just realized that there are already scripts to generate python docstrings from C++ files. (Previously I used LLM to extract these from documentation htmls, which seems to be totally unnecessary after reading the code base more.) The type stub file doesn't need to be pretty, so I'll work on an automated generation solution that generates the stub file then insert relevant Python docstrings to make it useful for the user (if not already inserted by 🙂 Will take a look at the |
Oh! Thanks for clarification. Found it to still be in |
I've successfully integrated the type stubs generation into the build process of ViZDoom! @mwydmuch A custom target
To test the build process, I have tested on an Apple Silicon Macbook Air via:
I have modified the documentation in |
Should be everything, got stub generation work as a standalone script as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your changes, I like most of them. However, I think I would prefer that generated_vizdoom_stubs.py be more of a utility tool/script (by default OFF in CMake) than a required part of the main building step.
Since stubs do not depend on the building process, IMO they should be committed to the repo, with the possibility to regenerate and to diff changes.
src/lib_python/CMakeLists.txt
Outdated
@@ -1,102 +1,114 @@ | |||
set(BUILD_PYTHON_VERSION "" CACHE STRING "Version of Python to build bindings for") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is all of this in diff? Could you modify only the necessary parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite strange, let me check whether it's line endings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original file had \r\n (Windows) line endings, while my uploaded file does not, I have added the \r\n endings back
To enable Python typing support, the creation of a Python Interface file `vizdoom.pyi` is now part of the build process. | ||
|
||
To ensure `vizdoom.pyi` is properly created, the following dependencies are required: | ||
* pybind11-stubgen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pybind11-stubgen
is needed as a part of the building process, then it should be added as build deps to pyproject.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it to pyproject.toml
- adding `import numpy as np` after `import typing` | ||
- optionally, `black` and `isort` for simple formatting | ||
- adding a header comment | ||
- replacing the `-> typing.Any` properties with actual return behaviour (`np.ndarray` or `typing.Optional[np.ndarray]`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct type annotation is actually numpy.typing.NDArrady
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to NDArray
examples/python/enum_typing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of having it as an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a leftover from when I used IntEnum as base-class for the Enum classes, I'll remove it
👍 I've made the requested changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the changes <3 LGTM!
Ok, merged, thank you @Trenza1ore. Great PR with very helpful stuff! |
Problem to solve
Currently ViZDoom (python binding) lacks any form of hinting with modern IDEs like VS Code due to the actual code being in a compiled binary (
vizdoom
,vizdoom.exe
, etc.)Proposed solution
A
vizdoom.pyi
file written according to official documentation at:Possible side effects
Not to my knowledge aside from being easier to work with :-)
Changed files
setup.py
: Addedvizdoom.pyi
andpy.typed
topackage_data
src/lib_python/CMakeLists.txt
: Added copy command forvizdoom.pyi
andpy.typed
src/lib_python/vizdoom.pyi
: New python interface filesrc/lib_python/py.typed
: New py.typed file for partial type checkingexamples/python/*
: Correction for type checkingtests/*
: Correction for type checkingBefore
After