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 support for positional args in pipeline.get_config() #2478

Merged
merged 2 commits into from
May 2, 2022

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Apr 29, 2022

Proposed changes:

  • support positional args in pipeline.config() (and thus YAML creation)
reader = FARMReader("my-model") # no model_name_or_path="my-model" required anymore
...
pipeline.add_node(name="reader", component=reader, inputs=["retriever"])

# YAML will contain reader's model_name_or_path param
pipeline.save_to_yaml(...)

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests

@julian-risch
Copy link
Member

@tstadel Is there a specific reason why we would like to support positional arguments? Throughout Haystack we are using named arguments and I think one important reason for that is that our frequent code changes could easily break things otherwise. Users should not rely on that the position of an argument will be the same in Haystack v1.5 etc. It's better to use the name explicitly no?

@tstadel
Copy link
Member Author

tstadel commented May 2, 2022

@julian-risch generally I agree, that keyword args are to be preferred. But this is more like a coding style topic and we should not enforce it in a strict sense for our users as long as positional args are commonly supported in python. Additionally we already had the problem within DC that not supporting positional args is confusing and we can expect users to expect them to work like in any other python lib.

TL;DR: the current implementation introduces more confusion than value and given that the support is cheap too we should opt for it.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thank you for explaining the motivation behind this PR. Looking into https://github.com/deepset-ai/haystack-hub-api/issues/611#issuecomment-1113275490 made it even easier for me to understand.

@tstadel tstadel merged commit 509944f into master May 2, 2022
@tstadel tstadel deleted the support_pos_args_in_yaml branch May 2, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants