-
Notifications
You must be signed in to change notification settings - Fork 15
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
[patch] Introduce a flag for _not_ mangling the name of wrapped function jobs #1356
Conversation
Then fall back on super() otherwise, to further clarify the intent of the mangling
The osx 12 tests failed, but it was a timeout and I have trouble seeing in the log but it looks like a completely unrelated test. I'm trying to just re-run, but the button doesn't do anything on my browser... /Users/runner/work/pyiron_base/pyiron_base/pyiron_base/jobs/datamining.py:651: PerformanceWarning:
your performance may suffer as PyTables will pickle object types that it cannot
map directly to c-types [inferred_type->mixed,key->block1_values] [items->Index(['name', 'array'], dtype='object')]
self.pyiron_table._df.to_hdf(
.
0%| | 0/4 [00:00<?, ?it/s]
100%|██████████| 4/4 [00:00<00:00, 41.70it/s]
TestWithProject: Setting up test project
.
The job test_a was saved and received the ID: 6
Loading and filtering jobs: 0%| | 0/5 [00:00<?, ?it/s]
The job test_b was saved and received the ID: 7
Loading and filtering jobs: 100%|██████████| 5/5 [00:00<00:00, 645.32it/s]
The job test_c was saved and received the ID: 8
The job test_d was saved and received the ID: 9
The job test_table was saved and received the ID: 10
Error: The action has timed out. |
Indeed, the same timeout error appears on #1349. It went through OK on #1352 though, so I'll probably try just re-running a few times... |
@@ -44,6 +44,8 @@ def __init__(self, project, job_name): | |||
super().__init__(project, job_name) | |||
self._function = None | |||
self._executor_type = None | |||
self._mangle_name_on_save = True # Automatically rename job using function and |
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.
self._mangle_name_on_save = True # Automatically rename job using function and | |
self._overwrite_job_name_by_hash = True |
I feel when we have to explain the variable name in a comment, then maybe the variable name is not intuitive.
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.
Yeah, I was aware of this deficiency -- I fixed it in the stacked PR, but it was still unprofessional of me to just leave it knowingly-bad here.
I find your suggestion loses information on when/why we would rename this, and focuses on the implementation (hashing) instead of the functionality (leveraging input values), so I opted to go with _automatically_rename_on_save_using_input
.
This is definitely an improvement, although in the stacked PR I'm still inclined to leave to comment to give even more depth to why we're doing this.
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.
In principle I agree with the changes, I just find the variable name _mangle_name_on_save
hard to understand and I do not understand why this information is stored in the HDF5 file.
Co-authored-by: Jan Janssen <[email protected]>
…_hashing_optional
That we broke by committing the readability suggestion
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.
Looks good to me
Uses a private interface and defaults false, so this is totally backwards compatible.