Refactor global state #486
Conversation
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
liamhuber
added
code_smell
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.
Instead of raw values from settings. A very good catch from @muh-hassani's review!!!
# Conflicts: # pyiron_base/job/generic.py
I'm seeing info.log instead of pyiron.log files cropping up. I think this is because log messages were happening before the logger was configured. So now that happens right at the beginning of pyiron_base.
These must have slipped in during the merge, oops
|
Last call for reviews. I had to resolve some merge conflicts earlier this week to keep this guy up-to-date...it didn't take long but I still don't want to have to do that again next week 😝 @niklassiemer @muh-hassani if you have concerns or really want some extra time to look at it lmk, otherwise I'll merge this evening. |
Co-authored-by: Niklas Siemer <[email protected]>
|
Build windows latest for py 3.8 hung indefinitely -- >2hrs, the rest of the builds, including windows on 3.9, finished in <<15 minutes. I'm restarting the build tests. |
Must have been cosmic rays; it ran fine the second time. |
There was a problem hiding this comment.
Sorry @liamhuber for the delay. Thanks for this careful refactory of the state. I just had one comment
| @@ -952,7 +950,7 @@ def run_if_scheduler(self): | |||
| self._logger.warning("Job aborted") | |||
| self.status.aborted = True | |||
| raise ValueError("run_queue.sh crashed") | |||
| s.logger.debug("submitted %s", self.job_name) | |||
| state.logger.debug("submitted %s", self.job_name) | |||
There was a problem hiding this comment.
Here state.logger and self._logger are basically the same!
There was a problem hiding this comment.
I guess self._logger is now redundant.
There was a problem hiding this comment.
Ah yeah, up above we have self._logger = state.logger. I guess I was a little too much on autopilot doing the refactor. I would actually suggest to eliminate self._logger in a housekeeping PR later. The extra layer of misdirection is not necessary in the code base, and it's a private attribute anyhow so no one's notebooks should be accessing the logger this way anyhow.
Awaiting #484.I want to break apart the settings object so its responsibility is clearer and more limited in scope. This was already done by breaking database access stuff off it (I forget the PR but it's
DatabaseManager). If we just do this, it means more and more imports, cf.Projectwhich now depends on both theSettingsandDatabaseManager. My solution is to make a newIDEstatesinglet helper class that acts as a single point of entry to all the other singlets (except maybeJobTypeChoice, which is anyhow on the chopping block).Still TODO in this PR:
FixScriptJoberror I introduced (at least that test is failing on my machine)Further rationalisation ofSettingsUnder-the-hood improvements to read and overwrite configuration in a clear wayUser input > env > specified config > default config, all XOR merging on top of the code defaultsStreamline user-facing stuff to aconfigurationattribute andupdatemethodShift responsibility for constructing database framework-specific connection strings from theSettingsover toDatabaseManagerExtract logger fromSettingsExtract queue adapter fromSettingsExtract publications fromSettingsAdd anupdatemethod tostatewhich updates the configuration and reinitializes the database and queue adapter stuff.Non-objectives for this PR:
DatabaseManageror database/filetable stuff. For now that the manager singleton can stay where it is.