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

Add target variables for 'enter-time' task #85

Closed
wants to merge 5 commits into from

Conversation

tudor-berariu
Copy link

Add a few more target vars.

@ppasupat
Copy link
Collaborator

Hi! Let me add a reviewer that can approve the pull request.

Comment on lines +45 to +46
return {query: q, queryText: queryText, expectedText: expectedQ};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Dedent these two lines.

<!DOCTYPE html>
<html>
<head>
<title>Find Word Task</title>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Update the title.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 -- not sure if we should include this at all, at least not in the same PR that puts the ground truth in JS variables.This enter-text-ocr is an additional task that requires entering text to help models learn OCR.

core.EPISODE_MAX_TIME = 20000; // set episode interval to 20 seconds

var genYear = function() {
return core.randi(1940, 2016).toString();
return core.randi(1940, 2016).toString();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Dedent the line.

@ppasupat
Copy link
Collaborator

@pseudo-rnd-thoughts This pull request edits the task HTML files by exposing task parameters as global JavaScript variables. This does not change the task behavior so it should be safe to merge. (Our Python API does not do anything with these variables, though.)

@mcobzarenco
Copy link

mcobzarenco commented Oct 16, 2023

Oops, we've made a mistake, we didn't mean to create the pull request here yet, but instead into our fork. Also, btw amazing job on maintaining the MiniWoB++ repo!

But given we did create the PR here, a bit of context. We have written policies by hand (using Playwright) to solve all the MiniWoB++ tasks by only interacting with the environment at pixel level (e.g. actions are Move(x, y), LeftClick, EmitText("Hello") etc.). An example:

000

You can use this to train an agent in a supervised fashion end-to-end or initialize policies with "behaviour cloning" for RL.

To do this, we found it easier to add the ground truth to JS variables for some tasks which we read from the env -- hence what's going on in this PR. If you are open to merging this upstream, we'd be very happy to clean up this PR a bit and resubmit it 🙇

@@ -21,7 +21,7 @@
// eventually ends the episode.
var translateCircle = function(translates) {
if(translates !== 0){
d3.select('#circ').transition().duration(2000)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes to chase-circle should also be removed

@pseudo-rnd-thoughts
Copy link
Member

@ppasupat Thanks for confirming, the PR is all your choice of what we do include, or not, etc, you know better about the project than I do

@ppasupat
Copy link
Collaborator

@mcobzarenco Thank you for the context. Since this PR is intended to be for a fork, would it be OK if we close the PR for now?

Adding ground truth to JS variables is a great idea though, and we would gladly accept an implementation. One possible improvement would be to make this exposed information uniform across all tasks. The Python API would also benefit from this (currently the fields in the observation are extracted using regular expression, which is not ideal).

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.

4 participants