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

Job factory call #495

Merged
merged 134 commits into from
Dec 14, 2021
Merged

Job factory call #495

merged 134 commits into from
Dec 14, 2021

Conversation

liamhuber
Copy link
Member

@liamhuber liamhuber commented Nov 3, 2021

During @jan-janssen's excellent thesis presentation (🥳 🥳 🥳 ) I really enjoyed seeing all the shiniest pyiron syntax, but during one of the loops I saw we still have pr.create_job('ScriptJob', 'foo'). This doesn't quite deprecate that, because we don't yet overload create with all the various toolkits, but it does let us do syntax like pr.base.job('ScriptJob', 'foo'), pr.atomistics.job('Lammps', 'bar'), etc, which is super convenient for loops over job types. And if/when we get create and create.job overloaded it should trivially extend to pr.create.job('SomeNewJobType', 'baz').

EDIT: Following @niklassiemer's suggestion you can also do pr.create.job(ScriptJob, 'foo') or pr.create.job(MyRandomChildOfGenericJob, 'bar').

And a bit of streamlining for handling the conda env resource paths
...now it seems to be working on Windows? But something else broke.
Also removed an old debug print I found in the archiving tests, which are, presumably, debugged.
And bask in the glory of having squashed a pain-in-the-ass OS-dependent test bug.
As far as I can tell the *only* place it was used was in the old tests
It doesn't seem to do anything.
The default file was always getting backed up, but previously if the test failed it never got restored.
Now that the settings singleton is not special, we only need one of them
Only leveraged it for the project so far. Also this introduced a bug in script jobs which are seeing weird settings, but someone it's *only* scriptjob that's having trouble
Ok, this is some deep magic I don't fully understand. Our Singletons are only single (if I understand) within a particular *python* instance. When I run the script job, it seems to be starting a *second* python instance with a *new* set of settings. I figured it out be changing the default behaviour to `project_check_enable=False` and seeing that it worked, but instead of messing with the defaults for now I'll just force the behaviour in the test.

So what's the deep magic about this? Well, for starters, if I run *only* the tests in `test_script.py` it works fine even without this patch, and it only fails when I run *all* the tests. This implies to me that script job is/isn't running on a new intpreter process depending where I start the tests from. Second, it worked *before*, so I have to assume that before it was somehow correctly reading the settings *even on this new python process*, and I really don't understand what I could have changed that stopped this from working.

My take home lesson is that we need to keep working on this corner of the code base, because we shouldn't be messing around with behaviour this complex, it's dangerous.
It would be a bit safer with @classmethod@property so these attributes couldn't get overwritten, but this is only possible in python >=3.9, and in the lower versions an equivalent implementation is hella ugly (https://stackoverflow.com/questions/128573/using-property-on-classmethods), so for now just live a little bit on the dangerous side.
Previously we stored strings which were the path to the module containing the job. That functionality is preserved here (although I don't think it's used anywhere anymore???) but we can also exploit a job class dict that's directly name:class.
# Conflicts:
#	pyiron_base/project/generic.py
@liamhuber liamhuber marked this pull request as ready for review November 12, 2021 19:49
@liamhuber
Copy link
Member Author

I'm a little confused why this is still showing up as so many commits...most of them got merged to master in #486, and master since got merged into this. Anyhow, the diff at least is correct that it's only a couple files changing.

@niklassiemer niklassiemer linked an issue Nov 26, 2021 that may be closed by this pull request
@liamhuber
Copy link
Member Author

In principle, for classes passed as strings, it could be re-written to make use of __getattr__ which in turn leans on JOB_CLASS_DICT so that we could pr.create.job('Lammps', 'lmp'). Right now this doesn't work since pyiron_base.job.factory.JobFactory._job_class_dict only knows about base jobs, while pyiron_atomistics.toolkit.JobFactory._job_class_dict only knows about atomistic jobs (see comment at top of PR). In practice JOB_CLASS_DICT is a weird global var we have floating around and I'm trying to phase it out. Since this is a bit of a niche ability anyhow, I'd rather lean on people directly calling pr.base.job, pr.atomistics.job etc. until we work out some sort of registration with pr.create.

@liamhuber liamhuber added the integration Start the integration tests with pyiron_atomistics/contrib for this PR label Nov 29, 2021
@stale
Copy link

stale bot commented Dec 13, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 13, 2021
@liamhuber liamhuber removed the stale label Dec 13, 2021
@liamhuber
Copy link
Member Author

@niklassiemer bump

@niklassiemer
Copy link
Member

Thanks, I was already bumped by the stale bot 😅

Copy link
Member

@niklassiemer niklassiemer left a comment

Choose a reason for hiding this comment

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

Other than the small nit about the job_name handling this LGTM.

pyiron_base/job/factory.py Outdated Show resolved Hide resolved
tests/job/test_factory.py Outdated Show resolved Hide resolved
tests/job/test_factory.py Outdated Show resolved Hide resolved
@niklassiemer
Copy link
Member

And I just realized a failing pyiron_atomistics compatibility test:

======================================================================
ERROR: test_run (atomistics.master.test_murnaghan_non_modal.TestMurnaghan)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\pyiron_base\pyiron_atomistics\tests\atomistics\master\test_murnaghan_non_modal.py", line 68, in test_run
    self.project.wait_for_job(murn, interval_in_s=5, max_iterations=50)
  File "C:\Miniconda\envs\test\lib\site-packages\pyiron_base\project\generic.py", line 1271, in wait_for_job
    wait_for_job(
  File "C:\Miniconda\envs\test\lib\site-packages\pyiron_base\server\queuestatus.py", line 200, in wait_for_job
    raise ValueError("Maximum iterations reached, but the job was not finished.")
ValueError: Maximum iterations reached, but the job was not finished.

I triggered a new execution.

Co-authored-by: Niklas Siemer <70580458+niklassiemer@users.noreply.github.com>
@liamhuber
Copy link
Member Author

Super thanks! Good catches; I'll add them tomorrow and merge (provided there is no further complication with the hung test)

@liamhuber
Copy link
Member Author

OSX failure:

======================================================================
302
FAIL: test_worker (job.test_worker.TestScriptJob)
303
----------------------------------------------------------------------
304
Traceback (most recent call last):
305
  File "/Users/runner/work/pyiron_base/pyiron_base/tests/job/test_worker.py", line 28, in test_worker
306
    self.assertEqual(len(df[df.status == "finished"]), 1)
307
AssertionError: 0 != 1
308

309
----------------------------------------------------------------------

I don't think either this nor the Murnaghan error are related to the changes here though.

@liamhuber
Copy link
Member Author

Indeed, it is definitely not. Further, I don't get that issue on my OSX machine.

@liamhuber
Copy link
Member Author

All tests passing after most recent master merge.

@liamhuber liamhuber merged commit 32ae587 into master Dec 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the job_factory_call branch December 14, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request integration Start the integration tests with pyiron_atomistics/contrib for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JobFactory should be callable
4 participants