Only calculate lengths of iterable arguments to map

[bunchesofdonald]

Summary

Issue #6383 calls out that a task with a default keyword argument fails when being mapped unless the task has defaulted that keyword argument to an unmapped value. This PR alters the way map works so that it only calculates the length of iterable arguments, which makes the use of unmapped un-needed in most cases.

This does result in a change to behavior. When map is called without any parameters, previously it would run the task a single time, now it will fail with a MappingMissingIterable exception. The thinking behind the exception is that if a user calls .map without an iterable then it's no different than calling the task directly and the user is either using an incorrect way of calling map or the arguments they're passing are incorrect.

Closes #6383

Steps Taken to QA Changes

• Unit tests
• Tested example code in Optional arguments must be explicitly passed with map #6383

Checklist

This pull request is:

• A documentation / typographical error fix
  • No tests or issue needed
• A short code fix
  • Please reference the related issue by including "closes <link to issue>" in this Pull Request's summary section.
    • If no issue exists, please create a bug report issue
  • Please include tests. One-line fixes without tests will not be accepted unless it's related to the documentation only.
• A new feature implementation
  • Please reference the related issue by including "closes <link to issue>" in this Pull Request's summary section.
    • If no issue exists, please create a feature enhancement issue
  • Please include tests
  • Please make sure that your QA steps are both thorough and easy to reproduce by somebody with limited knowledge of the feature that you are submitting

Happy engineering!

[zanieb]

This seems like really reasonable behavior, but I want to point out a change that I wasn't expecting. The original issue was that parameter defaults were not implicitly treated as unmapped e.g.

@task
def foo(x: int, y: int = 2)
    return x + y

foo.map(x=[1, 2, 3])  # Fails since `y` is not an iterable

The writer of the task should not need to account for it being used in mapping, hence this bug fix. However, this implementation comes with an interesting side-effect where the following will succeed as well:

@task
def foo(x: int, y: int = 2)
    return x + y + z 

foo.map(x=[1,2,3], y=10)

The user has explicitly provided a value for y but it is not mapped. I think this is a nice way to prevent the user from having to explicitly use unmapped in most cases! However, it might be confusing if they intended to pass an iterable and did not get a error when it was a scalar. I think this trade-off is worth it, but we should probably get feedback from @billpalombi

[bunchesofdonald]

Agreed, and I started down the route of implicitly marking the default arguments as unmapped (you can check out that version over here: 60f3d8b But it felt like we were just coding around our own limitations.

[zanieb]

Implementation LGTM. Can you explain this behavior in the map docstring? Is there a good place to clarify this behavior in the concepts?

[billpalombi]

Thanks for tagging me in @madkinsz! I agree that the trade off is worth it. This change is definitely an improvement over the current state. I'll keep a look out for users that are confused by the implicitly unmapped values, but it seems pretty intuitive to me.