8000 Add support for overriding list elements by vlthr · Pull Request #2585 · allenai/allennlp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Add support for overriding list elements #2585

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Add support for overriding list elements #2585

merged 3 commits into from
Mar 11, 2019

Conversation

vlthr
Copy link
Contributor
@vlthr vlthr commented Mar 9, 2019 •
edited
Loading

Currently, when lists are popped off Params, Params._check_is_dict() will give each element in the list a history of old_history.list.. If this history is later intended to be used to construct an overrides dict (such as inParams.add_file_to_archive()) this will cause the overridden dict to be constructed incorrectly:

from allennlp.common.params import with_fallback, unflatten

# dict resembling the one created by add_file_to_archive()
overrides_dict = {"old_history.list.some_value": 1}  

preferred = unflatten(overrides_dict) # => {"old_history": {"list": {"some_value": 1}}}
fallback = {"old_history": [{"some_value": 0}]}

config = with_fallback(preferred, fallback) # => {'old_history': {'list': {'some_value': 1}}}

Motivation

We use allennlp's config system to compose simple transformations on data, and these transforms occasionally require adding files to the archive.

Example (simplified, but hopefully illustrative):

{
    "dataset_reader": {
        "type": "transforming",
        "transforms": [
            {"type": "lowercase"}
            {"type": "normalize_patterns", "pattern_file": "file_to_be_archived.json"}
        ]
    }
}

Suggested fix

  1. Change Params._check_is_dict() to give list elements a history of old_history.{index}.
  2. Add the following case to with_fallback(): if a key exists in both the preferred and fallback dicts and fallback[key] is a list, treat preferred[key] as a sparsely represented list and use it to override selected indexes in fallback[key].

What do you think?

@joelgrus
Copy link
Contributor

I think this broadly looks good, you have a couple of pylint issues, can you fix them?

[17:49:36][Step 4/10] ************* Module allennlp.tests.common.params_test
[17:49:36][Step 4/10] C:346, 0: No space allowed before bracket
[17:49:36][Step 4/10]                 return cls(bs=[B.from_params(b_params) for b_params in bs] )
[17:49:36][Step 4/10]                                                                            ^ (bad-whitespace)
[17:49:36][Step 4/10] ************* Module allennlp.common.params
[17:49:36][Step 4/10] C:136, 0: Line too long (156/115) (line-too-long)
[17:49:36][Step 4/10] C:138, 0: Line too long (144/115) (line-too-long)

@joelgrus joelgrus merged commit 0f7bcf5 into allenai:master Mar 11, 2019
@joelgrus
Copy link
Contributor

thanks

reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
@vlthr @reiyw
Add support for overriding list elements (allenai#2585)
26ef08c 
* add support for overriding list elements

* fix formatting issues
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joelgrus joelgrus joelgrus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vlthr @joelgrus
0