-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Ported code to Gymnasium API #12
Ported code to Gymnasium API #12
Conversation
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.
Im very uncertain of the MiniWoBActionSpace
and MiniWoBObsSpace
.
Most training libraries require the use of Box
, etc from Gym / Gymnasium.
Is there no equivalent spaces in Gymnasium already?
Could we add CI test soon?
|
||
def __init__( | ||
self, | ||
subdomain, |
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.
Add type hints here, it is unnecessary to have the type hints in the docstring
for instance in self.instances: | ||
instance.wait() | ||
|
||
def reset(self, seed=None, options=None): |
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.
Add type hints
miniwob/environment.py
Outdated
else: | ||
return states, infos | ||
|
||
def step(self, actions): |
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.
Add type hints
Thank you for the code review.
The challenges are that the action space (1) has a dynamic size (e.g., can click on any element in the web page), and (2) : can be customized (e.g., one could add "drag" or "double click" actions). I can coerce the spaces to fit the predefined spaces, but it would require big code changes and reduce flexibility somewhat. Can I do this in a different pull request?
I can do this in a separate pull request, since it does not depend on the changes in this pull request. |
I think getting the spaces right should be a priority and I would prefer to avoid using this custom space. |
I see. Below is what I came up with for the action and observation spaces. What do you think? I would be happy to discuss further on Discord. Action space
Observation space
The original DOM dict (as returned by Javascript) will be put in the info dict |
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 that the changes mostly look good. Add the type hinting would be great.
The final thing to solve is the action and observation spaces. Could you check that the proposed spaces are actually instances of the current action / observation spaces. i.e., generate valid actions / observations for the environment and check all fit within the spaces.
Additionally, it would be nice to know what the flattened spaces look like as this is how most users would want to use the spaces.
When you have addressed the review comments, could you react to the comment in some way for me to know that you have solved it |
Ah, sorry, it's not done yet. I still need to add type annotations and verify the flattened spaces. I will mark the review comments as resolved when they're done. |
@pseudo-rnd-thoughts I have finished converting the action and observation spaces into Gymnasium build-in spaces. With this observation space, the seed determinism test also passes now. I also tried flattening the observation space: the top-level Dict couldn't be flattened since For modularity, can I address the type annotation in a separate pull request? I made a new GitHub Issue here: #14. |
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.
Thanks for making the changes to observation and action spaces.
This PR looks good to me
Description
Fixes #10. Ported the class
MiniWoBEnvironment
to thegymnasium.Env
interface. The environments are registered and a test is added to test the API.Note: This modification changes the signatures of methods in
MiniWoBEnvironment
. Old code using miniwob-plusplus will no longer work.Summary of the changes:
miniwob
package frompython/
to the root directory.MiniWoBEnvironment
to matchgymnasium.Env
.actions
,observations
, etc. become lists. This conflicts with the Gymnasium interface. The behavior has been changed to fit the Gymnasium interface. The old behavior can still be triggered using thenum_instances
argument.MiniWoBActionSpace
andMiniWoBStateSpace
. Currently, they are not extending existing spaces, and only check if the action/state are instances ofMiniWoBAction
/MiniWoBState
.test_api.py
) to test the code port.registration.py
)base_url
based on the file path. This removes the need to specify an environment variable.Type of change
Please delete options that are not relevant.
Checklist:
pre-commit
checks withpre-commit run --all-files
(seeCONTRIBUTING.md
instructions to set it up)