-
Notifications
You must be signed in to change notification settings - Fork 8
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
Implement self-attention and update utils #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice/only-slightly-scary. Only minor comments.
@DouglasOrr Added some changes. I think this is much less scary now 😱 |
"""run Python with the current directory on PYTHONPATH, for development""" | ||
run(["python"] + command) | ||
run(["python"] + list(command)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right. I've played around & I think you instead want to rename command
to commands
or in L166: rename "command" there, which is conflicting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused 🤔 Does my code error for you? If I run ./dev python
it works fine.
If I change it to:
def python(*commands: Any) -> None:
"""run Python with the current directory on PYTHONPATH, for development"""
run(["python"] + commands)
I'm getting:
...
run(["python"] + commands)
TypeError: can only concatenate list (not "tuple") to list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry I meant the main thing is to drop the varargs *
.
I think with the currently committed version maybe ./dev python
works but ./dev python my_script.py
doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I forgot what it looked like before. If it's ok I'll make this change in the next pr (makes my life slightly easier with git 🙃 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; only one minor comment for dev, but nothing blocking / pre-approved from here in any case.
(And, yes, this is much less scary; looks grand!)
No description provided.