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.

Sign up for GitHub

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

Jump to bottom

Spielviz gives AttributeError: module 'pyspiel' has no attribute 'GameParameter' #1224

Open
GeorgeBreahna opened this issue May 10, 2024 · 28 comments
Open

Spielviz gives AttributeError: module 'pyspiel' has no attribute 'GameParameter' #1224

GeorgeBreahna opened this issue May 10, 2024 · 28 comments

Comments

@GeorgeBreahna
Copy link

Looks like at some point open_spiel lost compatibility with spielviz due to changes to how GameParameter is compiled in pyspiel. Another user brought it up in a discussion and I have the same problem, so I'm making it an Issue for visibility.

Other user's description:

Hi, I encountered some difficulties while trying to run SpielViz after installing it. When I tried to run it, I kept receiving an error message: AttributeError: module 'pyspiel' has no attribute 'GameParameter'
Is this caused by a version update of OpenSpiel? If that's the case, which version should I use to adapt to SpielViz?

Originally posted by @light188 in #1186 (reply in thread)
@tacertain
Copy link
Contributor

I think you need to build from https://github.com/google-deepmind/open_spiel/releases/tag/v0.3.1. I'm guessing that's going to be challenging, though, given all the dependencies you'll have to get old versions of.

Here's the commit that removes it: 8852909, which was cleanup from f5df897

My guess is that you'll have better luck updating SpielViz.

@lanctot
Copy link
Collaborator

lanctot commented May 10, 2024

Hi, great thanks, yes -- this is a bug in SpielViz not open_spiel. It needs to be fixed on their end. Can you please open the same bug on the SpielViz page instead? I might even fix it myself.. should be a very easy fix.

@lanctot lanctot closed this as completed May 10, 2024
@GeorgeBreahna
Copy link
Author

Thanks for the quick response! The same issue was opened over a year ago on their end: michalsustr/spielviz#3. Spielviz seems somewhat dead though, but I don't have a good alternative for exploring large games (I planned to use it for reality checking my implementation of some games).

@tacertain
Copy link
Contributor

I'll see if I can figure it out while I'm waiting for @lanctot to look at my other PR. 😉

Though I guess this might disincentivize his looking at it until I fix this one. 🤔

@GeorgeBreahna
Copy link
Author

I've tried building pyspiel from various older versions of open_spiel before opening this issue, but it either had the same problem (no GameParameter) or I encountered compiler errors. Specifically with v0.3.1, both g++ 12.2.0 and clang 14.0.6 complain about "invalid use of incomplete type ‘PyFrameObject’". One example from pybind11/include/pybind11/cast.h (my inline comments):

#if !defined(PYPY_VERSION)
    if (scope.trace) {
        auto *trace = (PyTracebackObject *) scope.trace;

        /* Get the deepest trace possible */
        while (trace->tb_next)
            trace = trace->tb_next;

        PyFrameObject *frame = trace->tb_frame;                  // init
        errorString += "\n\nAt:\n";
        while (frame) {
            int lineno = PyFrame_GetLineNumber(frame);
            errorString +=
                "  " + handle(frame->f_code->co_filename).cast<std::string>() +    // error
                "(" + std::to_string(lineno) + "): " +
                handle(frame->f_code->co_name).cast<std::string>() + "\n";     // error
            frame = frame->f_back;    // error
        }
    }
#endif

Also from pybind11/include/pybind11/pybind11.h

PyFrameObject *frame = PyThreadState_Get()->frame;    // error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘frame’; did you mean ‘cframe’?

These source files are created after running ./install.sh so I assume this is caused by bad dependency?

@tacertain
Copy link
Contributor

I dunno. That release is three years old and reproducible builds does not seem to be a priority for open spiel. You'd need to figure out all the dependent libraries and make sure they are all at the right version.

Usually the best path is forward.

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

@GeorgeBreahna please don't use OpenSpiel 0.3.1.

It will be a 2-3 line fix in SpielViz. If you point me to the lines in SpielViz that use pyspiel.GameParameter I can even give you the fix by tomorrow.

@GeorgeBreahna
Copy link
Author

Spielviz uses it only in spielviz/ui/views/game_information_view.py line 120.

@tacertain
Copy link
Contributor

Try this: https://codeload.github.com/tacertain/spielviz/zip/refs/heads/master

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

Removing the pytype hints should be enough.

Change:

  def _update_param_grid_rows(self, grid: Gtk.Grid,
                              params: Dict[str, pyspiel.GameParameter]):

to

  def _update_param_grid_rows(self, grid: Gtk.Grid, params):

or

  def _update_param_grid_rows(self, grid: Gtk.Grid,
                              params: Dict[str, Any]):

I don't remember why we removed GameParameter from the python binding but we should certainly add it back (specifically for this reason, that we should support the pytype hints)

@locked do you remember?

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

Thanks for the quick response! The same issue was opened over a year ago on their end: michalsustr/spielviz#3. Spielviz seems somewhat dead though, but I don't have a good alternative for exploring large games (I planned to use it for reality checking my implementation of some games).

I know the author and will politely prod him... :)

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

Thanks for the quick response! The same issue was opened over a year ago on their end: michalsustr/spielviz#3. Spielviz seems somewhat dead though, but I don't have a good alternative for exploring large games (I planned to use it for reality checking my implementation of some games).

I know the author and will politely prod him... :)

But I think this will get fixed once we add back GameParameter. I'll make sure v1.5 has this (which should be released in the next few weeks).

Re-opening this issue as a reminder to submit this fix on our side.

@lanctot lanctot reopened this May 11, 2024
@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

I think longer term someone should either takeover SpielViz maintenance or make a new visualization tool. I would be quite happy with a new one that is not licensed under GPLv3. It could even been a nice project to include in the main OpenSpiel repos itself.

@tacertain
Copy link
Contributor

@lanctot Looking at the original commit, it seems to me that the GameParameter type was removed so that python code can just use native python types when interacting with the framework instead of having to cast stuff back and forth. There was also a GameParameter method that allowed access to the parameter dictionary by passing in a key and getting back the corresponding parameter value. Adding the method back would indeed be trivial, but I don't think that's what's breaking spielviz. It seems to me that going back to the OpenSpiel-specific types rather than the python native types might be a step in the wrong direction. I don't think there's any functionality lost as you can use python type introspection to determine the types if necessary.

But I might be completely misunderstanding the code and/or your suggestion.

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

I didn't really have a suggestion.. I would have to dig into the code to understand things better first. It's been a while since I looked at that.

But I would like SpielViz (or anything else) to be able to use type hints for game parameters. (And, are you saying that they are all native python types?) Does your solution above do that?

@GeorgeBreahna
Copy link
Author

Removing the type hint did help it, but is_mandatory() is called on it, which is missing (line 94 of the same file):

label.set_text(f"{name}*" if default_param.is_mandatory() else name)

I changed it to label.set_text(name) to avoid the condition, though obviously I'm completely ignorant to why the condition was necessary. The program starts normally.

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

@tacertain

Actually, I guess I did have a suggestion, sorry. I think you're saying that we now use python native types for the game parameters, and if so.. I agree that's probably the way to go (rather than bringing back the OpenSpiel-specific type).

But then we could define a type GameParameter to be something like:

GameParameter = int | str | bool | float

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

Removing the type hint did help it, but is_mandatory() is called on it, which is missing (line 94 of the same file):

label.set_text(f"{name}*" if default_param.is_mandatory() else name)

I changed it to label.set_text(name) to avoid the condition, though obviously I'm completely ignorant to why the condition was necessary. The program starts normally.

Cool! That should work. @michalsustr was just being fancy with the names.

(OpenSpiel parameters could be optional or mandatory and he was adding a star to the mandatory ones.)

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

Awesome progress, thanks! It would be great if one of you submitted a PR to SpielViz to fix it.

I would then happily volunteer to send @michalsustr one email per day until it's merged. :-p

@tacertain
Copy link
Contributor

tacertain commented May 11, 2024 via email

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

Yeah, I am just not sure if it fixes it without trying it out, and won't be able to until Monday. Is it easy for you to try it? If you do and it works, can you submit a PR on our side? 🙏

@tacertain
Copy link
Contributor

tacertain commented May 11, 2024 via email

@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024

Good point, yeah we'll need to support Python 3.9. I'll take a crack at it and worst case we just leave the fix entirely to the SpielViz side.

@lanctot lanctot mentioned this issue May 11, 2024
Merged
@lanctot
Copy link
Collaborator

lanctot commented May 11, 2024 •
edited

Alright! Some flyby hacking brought to you directly from LAX.. #1226

Did one of you build from source? (@tacertain I think you did, not sure about @GeorgeBreahna) Can you try applying this two-line change above and then, and then in SpielViz add this to the top of the game_information_view.py file:

from open_spiel.python import GameParameter

And then put back that pytype hint on line 120, and finally check if SpielViz does not crash with the type in that case?

@tacertain
Copy link
Contributor

tacertain commented May 11, 2024 via email

@tacertain
Copy link
Contributor

It works if I change the pytype hint to plain GameParameter from pyspiel.GameParameter (unsurprisingly)

@lanctot
Copy link
Collaborator

lanctot commented May 13, 2024

It works if I change the pytype hint to plain GameParameter from pyspiel.GameParameter (unsurprisingly)

Ok, great. I've now merged #1226 so it'd be great if one of you could submit a PR fix to SpielViz and I'll contact the author to merge it.

@lanctot lanctot mentioned this issue May 13, 2024
Open
@tacertain
Copy link
Contributor

michalsustr/spielviz#4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tacertain @lanctot @GeorgeBreahna