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

Move argument parsing to MLBlock #86

Closed
csala opened this issue May 16, 2019 · 0 comments · Fixed by #87
Closed

Move argument parsing to MLBlock #86

csala opened this issue May 16, 2019 · 0 comments · Fixed by #87
Assignees
Labels
internal improvement This introduces no noticeable changes in the behavior but improves code quality and performance
Milestone

Comments

@csala
Copy link
Contributor

csala commented May 16, 2019

The current version of MLBlocks is preparing the keyword arguments for the MLBlock, including keyword replacement and the lookup for a default value if an argument is missing inside the MLPipeline.

The consequence of this is that if a primitive has defined an argument name different than the keyword expected by the actual primitive method, the MLBlock instance expects to be called with the actual primitive keyword instead of the one defined in the primitive JSON.

An example of this can be seen in the numpy.argmax primitive:

The JSON contains:

    "produce": {
        "args": [
            {
                "name": "y",
                "keyword": "a",
                "type": "ndarray"
            }
        ],
        ...
    }

As a consequence of this, the block is exposing the argument name as y:

>>> block = MLBlock('numpy.argmax')
>>> block.produce_args
[{'name': 'y', 'keyword': 'a', 'type': 'ndarray'}]

But, since the keyword argument is not being parsed by the MLBlock class itself, if we call it using the y keyword it fails:

>>> block.produce(y=[[1, 2, 3]])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/xals/Projects/MIT/MLBlocks/mlblocks/mlblock.py", line 310, in produce
    return self.primitive(**produce_kwargs)
TypeError: argmax() got an unexpected keyword argument 'y'

And the same would happen if there was a default value defined.

To fix this, we can move the parsing logic from the MLPipeline to the MLBlock, so that the block methods can also be called with the exposed argument names and also have default values.

@csala csala self-assigned this May 16, 2019
@csala csala added the internal improvement This introduces no noticeable changes in the behavior but improves code quality and performance label May 16, 2019
@csala csala added this to the 0.3.1 milestone May 16, 2019
@csala csala closed this as completed in #87 May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal improvement This introduces no noticeable changes in the behavior but improves code quality and performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant