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

give name of function inside incorrect parameters error #568

Merged
merged 34 commits into from
Oct 22, 2020

Conversation

WaylonWalker
Copy link
Contributor

@WaylonWalker WaylonWalker commented Oct 17, 2020

Description

I messed up my author config, I rebased and opened this new PR similar to #538

This is a quick and simple step towards the right direction to resolve #495.

Development notes

I ran kedro new, and broke one of the nodes. There is a comparison of the current error and the updated error below. This error is also inspected by the test framework.

Current Error

Does not give much information to what node caused the TypeError. While this is generally easy to figure out on small pipelines, larger pipelines can be quite intense to figure out where the error came from. Any additional information would be greatly appreciated.

image

Updated Error

The updated error will give the name of the function for full functions, but does not provide better messaging for lambdas.

image

Feedback

  • Is the format of the message ok?
  • Is there any potential that the passed in function would not have a func.__code__.co_name method?

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change and added my name to the list of supporting contributions in the RELEASE.md file
  • Added tests to cover my changes
  • This PR is on my personal behalf and does not represent any employer

NOTE No new tests were created, only modification of existing tests.

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

conversation from #538


@WaylonWalker : @lorenabalan any ideas how to get tests passing? For some reason 58636d9 was only failing on 3.8. It appeared to match exactly to me besides the escape characters.

@lorenabalan : Actually looking at the test cases, I would argue the error message looks more complicated now - addresses in memory are not that user-friendly. Could we instead factor out and leverage the same logic we have in Node._func_name? 😃

@WaylonWalker : I am not sure that my change should be causing a seg fault in the memory profiler tests???
Could you rerun tests and see if it persists?
I think I got it cleaned up, and it looks much better, than for the _func_name suggestion @lorenabalan!

@WaylonWalker : Thanks for re-running! Tests are passing. @lorenabalan how does the updated format look? I like that it matches up with other kedro function naming methods.
Personally I am not sure that I like that there are more than one place figuring out how to name a function, but that would have started touching more things than I preferred to do from my end without first discussing.

@lorenabalan : Thanks for re-running! Tests are passing. @lorenabalan how does the updated format look? I like that it matches up with other kedro function naming methods.
The format looks good to me - maybe could be Inputs of "this_func_name" function instead of Inputs of "this_func_name" in the error message but that's a minor point.
Personally I am not sure that I like that there are more than one place figuring out how to name a function, but that would have started touching more things than I preferred to do from my end without first discussing.
Yeah I agree - looking through the code base, I also found this variation of the implementation. 😅 The decorators are going to be deprecated in 0.17.0 so I'm not that worried about that one, it just illustrates that we don't have a consistent way to handle it. Could we factor out the logic from _func_name into a helper function _get_readable_func_name or smth, to call in both places (in _func_name and in _validate_inputs)? And only warn about update_wrapper in Node._func_name.

@WaylonWalker WaylonWalker requested a review from idanov as a code owner October 17, 2020 16:06
@WaylonWalker WaylonWalker changed the title Correct email give name of function inside incorrect parameters errorCorrect email Oct 17, 2020
@WaylonWalker
Copy link
Contributor Author

@lorenabalan I implemented the change to move _func_name logic to a helper function _get_readable_func_name . And referenced it in bother places rather than duplicating. Let me know your thoughts. Also heads up, I messed up my git author and didn't want duplicate commits so I opened a new PR.

Looks like I ran into the same segfault/timeout error in testing. Can you rerun them?

@WaylonWalker WaylonWalker changed the title give name of function inside incorrect parameters errorCorrect email give name of function inside incorrect parameters error Oct 19, 2020
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with this! Nearly there.

Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

Awesome! Can you please remove temp1 file? 😅 I think it's been accidentally committed at some point.

Copy link
Contributor

@DmitriiDeriabinQB DmitriiDeriabinQB left a comment

Choose a reason for hiding this comment

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

Nice! 👍

WaylonWalker and others added 4 commits October 21, 2020 08:01
We try follow a convention where all f-string parts are prefixed with f even if there are no variables inside. It just for the consistency with how PyCharm, for example, breaks down such strings.

Co-authored-by: Dmitrii Deriabin <[email protected]>
@WaylonWalker
Copy link
Contributor Author

Thanks for the help @DmitriiDeriabinQB ! All passing now.

@lorenabalan lorenabalan merged commit cca6973 into kedro-org:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

better error messages when wrong number of inputs is passed
3 participants