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

Lock ability to create new fields #147

Open
aronchick opened this issue Apr 8, 2020 · 6 comments
Open

Lock ability to create new fields #147

aronchick opened this issue Apr 8, 2020 · 6 comments

Comments

@aronchick
Copy link

Is it possible to freeze new field creation via dot access without freezing the underlying fields to be immutable?

@cdgriffith
Copy link
Owner

cdgriffith commented Apr 9, 2020

I was actually thinking of this in a different way earlier this week. From the standpoint of having a Schema that a box had to adhere too. I personally decided against it as the best schema validation tool I know of for this would be jsonschema and it really has such simple operation it makes more sense to use it by itself than incorporate it into box.

However that is an interesting way to go about it is just not allowing more fields after initial creation. So the question then would be what about sub boxes, would they by default also be non-mutable, but their values be mutable? and then you could just upright replace a sub-box with a new one with different keys.

@aronchick
Copy link
Author

Well, perhaps I'm not thinking about this properly :)

I'm inheriting from Box to create a subclass - MLBox - which is of a specific type. So, for the sake of argument, let's say I want to do the following (forgive the hacky code):

ml_dict = { 'schema': 'model_schema', 'framework': 'tensorflow', 'version':'2.0', 'model_package': { 'URI': 'https://s3.amazon.com/somebucket/model.pkl', 'model_sha': 'ea4963009b2c0f5b476984e024cbe66f5a8355d0'} }

# For a bunch of reasons, I need to split the instantiation of the Box from the setting of the dict from the loading of the dict
my_model_object = MLBox()

# Builds the empty schema - so all the keys above but set to "None"
my_model_object.set_schema('0.0.1', 'model_schema')

# Below works
my_model_object.framework = 'foo'

# Should throw keyerror, not create a new field
my_model_object.bar = 'qaz'

# below will overwrite anything already in the box
my_model_object.load(ml_dict)

Does this make sense?

As an aside, is there a blessed way to set all the fields AFTER the box has been instantiated? Or I'm just using self.merge_update(ml_dict) inside the .load(dict) function above, which feels fine, but wanted to make sure.

@cdgriffith
Copy link
Owner

That sounds like a reasonable way to approach it. It is not something outside the realm of possible, but would probably be a heavy addition / it's own inherited box type (like how ConfigBox or SBox are currently). Will admit not a high priority item myself, but would welcome PRs for it.

To achieve a similar thing now, I would still suggest using jsonschema so you could make said schema as you defined, and then before wherever the my_model_object will be used do a validate against it to make sure it's sane first.

As far as updating or setting fields the goal of Box is to be a pure dict drop in, so any of those methods should work as expected. Then merge_update is a "kinder" update that will merge sub Boxes/dicts together instead of overwriting what is already there. So that is more of a design choice. So yes, if that is the goal, that sounds like a good choice to me.

@aronchick
Copy link
Author

Sounds good! Do you want me to leave this open as an enhancement?

And so, to be clear, is there anything I can do to allow for the freezing just of new field creation? (e.g. is there a method I should subclass in order to do this? or is it effectively impossible)?

@cdgriffith
Copy link
Owner

cdgriffith commented Apr 10, 2020

Yes please leave it open as an idea.

Easiest way I can think of would be to subclass box and the __setitem__ method to do a check of

if self._box_config["__created"] and key in self:
    raise BoxError("Cannot create new fields")

@tikendraw
Copy link

Just saying. Use Pydantic. It accepts dicts as input, parses as your intent and has configuration. I liked the idea of dict accessible with dot notation. It is now try to be pydantic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants