-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 custom python cell magics #2744
Changes from 12 commits
3a6c30e
65d48e4
f469c88
f0c67bc
8110734
f18fc2b
7b3f7ab
5eb71d8
00e02c5
e23dc2f
924e900
96cf858
d004e5a
8b12ba3
c495217
376dfe3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,6 +130,7 @@ class Mode: | |
is_ipynb: bool = False | ||
magic_trailing_comma: bool = True | ||
experimental_string_processing: bool = False | ||
python_cell_magics: Set[str] = field(default_factory=set) | ||
|
||
def get_cache_key(self) -> str: | ||
if self.target_versions: | ||
|
@@ -147,5 +148,6 @@ def get_cache_key(self) -> str: | |
str(int(self.is_ipynb)), | ||
str(int(self.magic_trailing_comma)), | ||
str(int(self.experimental_string_processing)), | ||
",".join(sorted(self.python_cell_magics)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point I'm a little worried that we might hit some filesystem file name length limits. I assume that Python cell magics are also filesystem safe? can't contain slashes, colons, whatever isn't safe. Worst case we could hash 'em with blake2/3 or something. The cache should fail gracefully but I'm not entirely sure if it does so yeah. This can be resolved separately though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Maybe we can hash the value with some consistent hashing algorithm available in the stdlib. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, in general, most magics are simply function names and so must abide by the corresponding python naming requirements. But I think there are also ways to generate the magics labeled with an arbitrary string. Does the cache label need to be human readable? If not, +1 to hashing. You can also ensure that the name has a set length, and so could apply this to the entire cache string. We use the following code when hashing arbitrary length strings for file-system caching: hashlib.md5(label.encode()).hexdigest() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't need to be human-readable, though it's helpful for debugging if it's not too bad. The other components of the key should mostly just be ints, so I think we can get away with not hashing them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you make the change to hashing? Also, there are a few merge conflicts now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course. I've done that and resolved the merge conflicts. |
||
] | ||
return ".".join(parts) |
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.
A bit jarring to write
--python-cell-magics custom1 --python-cell-magics custom2
. Should we use a comma-separated list instead?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.
Agreed, though this is how
click
supports multiple options out of the box. This is also howtarget-version
takes multiple arguments. I expect that we would need to introduce a custom callback to support multiple options, which might then also require custom logic when reading from thepyproject.toml
file.My expectation is that this will rather mostly be used in a configuration file and so perhaps the clunkiness on the command line is acceptable.
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.
That's fair!