Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

1. Shell and python file to only download image chat data. 2. Updated… #2381

Merged
merged 8 commits into from
Feb 7, 2020

Conversation

shubhamagarwal92
Copy link
Contributor

@shubhamagarwal92 shubhamagarwal92 commented Feb 4, 2020

Patch description

  1. Shell and python file to only download image_chat data.
  2. Updated Readme for the same.

The current setup to download just the data is cumbersome requiring full knowledge of ParlAI platform. Making it easy for researchers who just want to use the data and build their code on top of different repos. Basically a direct api for the build in parlai.tasks.image_chat.build

Testing steps
Enter steps to test your pull request. Give a clear and concise description of
what you expected to happen during testing.

Run as ./parlai/tasks/image_chat/download_data.sh to download only data. (Updated the README.)

Logs

Running code from: 
Downloading in data root:
[ downloading: http://parl.ai/downloads/image_chat/image_chat.tgz to .....]

Other information
@klshuster please verify if this is the correct data.

@stephenroller
Copy link
Contributor

Thanks for your contribution. The unit tests failures look transient, so I've hit rerun. Definitely please fix the lint error. I'll leave it to @klshuster to evaluate the contents of the PR.

Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

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

Looks good to me. The lint error is that the download_data.py file is missing our copyright header (you can take a look at any other .py files in the repo to copy over the appropriate info). Failing test indeed looks transient; will wait on full approval once lint is taken care of.

Thank you for your contribution!

parlai/tasks/image_chat/download_data.sh Outdated Show resolved Hide resolved
@shubhamagarwal92
Copy link
Contributor Author

Hi @klshuster

I have added the copyright and run pylint. Please find the output now

pylint download_data.py

************* Module parlai.tasks.image_chat.download_data
download_data.py:1:0: C0114: Missing module docstring (missing-module-docstring)
download_data.py:29:4: C0103: Constant name "opt" doesn't conform to UPPER_CASE naming style (invalid-name)

------------------------------------------------------------------
Your code has been rated at 8.18/10 (previous run: 6.36/10, +1.82)

@klshuster
Copy link
Contributor

could you please run bash autoformat.sh in the ParlAI repo and then push those changes?

@shubhamagarwal92
Copy link
Contributor Author

@klshuster Seems like autoformat doesn't like the location of the files. I don't know what is happening under the hood but this commit cd8a0e4 shows that yml files have somehow been changed, due to which the tests are failing.

Do you want me to manually correct the yml and md files?

@klshuster
Copy link
Contributor

yes, could you please revert anywhere where it changed "setup.py" to "download_data.py"

@klshuster
Copy link
Contributor

Not sure why autoformat.sh won't work for you, however here are the lint changes to make, then we should be good to merge:

 def parse_args():
     """
-    Wrapper to parse CLI arguments
+    Wrapper to parse CLI arguments.
+
     :return: args
     """
     parser = argparse.ArgumentParser()
     parser.add_argument(
-        "-dp", "--datapath", default="/tmp",
-        help="Path where to save data."
+        "-dp", "--datapath", default="/tmp", help="Path where to save data."
     )

@shubhamagarwal92
Copy link
Contributor Author

Thanks for your suggestion. :)

Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for updating!

@klshuster
Copy link
Contributor

I'll go ahead and merge this

@klshuster klshuster merged commit 917baab into facebookresearch:master Feb 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants