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

[minor] Opt in hash for pythonfunction #1358

Merged
merged 25 commits into from
Mar 7, 2024
Merged

Conversation

liamhuber
Copy link
Member

This changes the default behaviour of PythonFunctionContainerJob to not automatically rename jobs at save time; Project.wrap_python_function still does by default, but it's now optionally exposed as a flag.

This extends and polishes the ideas in #1356, but I wanted to break it apart separately since changing the default behaviour for the job class probably makes this a minor bump instead of a patch bump.

@liamhuber
Copy link
Member Author

This percolated through the day, and I realized that in the wrapper function it would be much nicer to expose job_name: Optional[str] = None than the Boolean flag. Default would still be exactly the function name and input hash, but there's no reason to stop people from assigning a specific job name they want!

There's no time pressure for this PR, so I'll polish it on Monday.

@liamhuber liamhuber self-assigned this Feb 24, 2024
@liamhuber
Copy link
Member Author

This percolated through the day, and I realized that in the wrapper function it would be much nicer to expose job_name: Optional[str] = None than the Boolean flag. Default would still be exactly the function name and input hash, but there's no reason to stop people from assigning a specific job name they want!

There's no time pressure for this PR, so I'll polish it on Monday.

Tuesday, but anyhow done. You can force the wrapped job to have exactly the name you want like:

job = pr.wrap_python_function(
    some_function,
    job_name="my_job",  # Overrides default of using function name
    automatically_rename=False,  # Stops name mangling with input at save time
)

pyiron-runner and others added 2 commits March 4, 2024 19:37
# Conflicts:
#	pyiron_base/jobs/flex/pythonfunctioncontainer.py
#	tests/flex/test_pythonfunctioncontainer.py
@liamhuber liamhuber requested a review from jan-janssen March 4, 2024 19:42
@liamhuber liamhuber marked this pull request as ready for review March 4, 2024 19:42
Base automatically changed from make_hashing_optional to main March 7, 2024 17:26
@liamhuber liamhuber added the format_black reformat the code using the black standard label Mar 7, 2024
@liamhuber liamhuber marked this pull request as draft March 7, 2024 17:42
Copy link
Member

@jan-janssen jan-janssen left a 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

@liamhuber liamhuber marked this pull request as ready for review March 7, 2024 17:52
@liamhuber
Copy link
Member Author

Looks good to me

Super! I'm just adding a bit to the tests to improve coverage -- most of the codacy report is somehow catching old stuff that has nothing to do with this PR, but they did find a relevant branch of an if-clause that should get a quick test.

@liamhuber
Copy link
Member Author

Ok, so maybe this branch is honestly not doing what I hoped it would do. This is exactly #1367 cropping up, and I thought this clause stopped it.

ValueError: job name 'my_function' in this project 'tests/flex/test_pythonfunctioncontainer/' is not unique '[{'id': 8, 'parentid': None, 'masterid': None, 'projectpath': '/home/runner/work/pyiron_base/pyiron_base/', 'project': 'tests/flex/test_pythonfunctioncontainer/', 'job': 'my_function', 'subjob': '/my_function', 'chemicalformula': None, 'status': 'created', 'hamilton': 'PythonFunctionContainerJob', 'hamversion': '0.4', 'username': 'pyiron', 'computer': 'pyiron@fv-az575-929#1', 'timestart': datetime.datetime(2024, 3, 7, 17, 52, 55, 710069), 'timestop': None, 'totalcputime': None}, {'id': 9, 'parentid': None, 'masterid': None, 'projectpath': '/home/runner/work/pyiron_base/pyiron_base/', 'project': 'tests/flex/test_pythonfunctioncontainer/', 'job': 'my_function', 'subjob': '/my_function', 'chemicalformula': None, 'status': 'created', 'hamilton': 'PythonFunctionContainerJob', 'hamversion': '0.4', 'username': 'pyiron', 'computer': 'pyiron@fv-az575-929#1', 'timestart': datetime.datetime(2024, 3, 7, 17, 52, 55, 762810), 'timestop': None, 'totalcputime': None}]

@liamhuber
Copy link
Member Author

Ahhh, right, this is actually perfectly sensible. The "don't save again if you find this job" clause is nested inside the if self._automatically_rename_on_save_using_input, but I was testing it when the name was not being automatically changed so of course it got missed!

Since the consensus in #1367 seems to be that we want all jobs to have this protection, I'll just de-indent the clause here so (as a start) this job always has the protection.

Apart from making the coverage number better, this was actually my motivation for adding the test to start with -- I anticipate this snippet getting promoted so I wanted a test that could be pulled along with it.

@liamhuber liamhuber merged commit 8d187c8 into main Mar 7, 2024
23 of 25 checks passed
@liamhuber liamhuber deleted the opt_in_hash_for_pythonfunction branch March 7, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants