Skip to content

ENH: Workspace: Suspend attribute dependency updates during init#554

Merged
richardotis merged 12 commits intopycalphad:developfrom
richardotis:wks-init-order
Aug 6, 2024
Merged

ENH: Workspace: Suspend attribute dependency updates during init#554
richardotis merged 12 commits intopycalphad:developfrom
richardotis:wks-init-order

Conversation

@richardotis
Copy link
Collaborator

@richardotis richardotis commented Aug 3, 2024

This PR changes how Workspace objects are initialized, to ensure that each attribute is initialized after its dependencies (database before models, parameters before phase_record_factory, etc). It also suspends dependency callbacks inside of Workspace.__init__ to avoid calls to the default_factory of attributes that will be overwritten again later in initialization.

For the worst case of a user passing in fully-initialized attributes to the constructor of a Workspace object, with pre-computed models and phase_record_factory (a common case inside of ESPEI and any other software calling pycalphad in a high-throughput loop), this PR should substantially improve performance.

We can be a little smarter still, by eliding dependency updates for cases where the change in the dependency doesn't actually affect the dependent object; for example, phase_record_factory only really depends on the keys of conditions, not the values, so we check for that in PRFField.on_dependency_update and avoid rebuilding it if they haven't changed.

@codecov
Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 98.64865% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.85%. Comparing base (176909c) to head (aef2abf).
Report is 1 commits behind head on develop.

Files Patch % Lines
pycalphad/codegen/phase_record_factory.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #554      +/-   ##
===========================================
+ Coverage    91.78%   91.85%   +0.06%     
===========================================
  Files           77       77              
  Lines        11996    12086      +90     
===========================================
+ Hits         11010    11101      +91     
+ Misses         986      985       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richardotis
Copy link
Collaborator Author

richardotis commented Aug 3, 2024

In a departure from best practice, I'm including a few unrelated commits here (related to errors when phase_record_factory.keys() is not equal to Workspace.phases) that fix a bug encountered downstream in ESPEI. I'm putting them here because all of these commits are needed for ESPEI's compatibility with Workspace, and it's easier for testing if it's all contained within one branch.

@richardotis
Copy link
Collaborator Author

Windows test failures are due to matplotlib/matplotlib#28551 (comment)

@tacaswell
Copy link

The 3.9.1 wheels on windows would, depending on import order, cause segfaults, see matplotlib/matplotlib#28551 (comment).

We deleted the bad wheels to avoid segfaults (which seemed like a good idea) but failed to anticipate that this would cause downstream projects to start trying to build mpl from source instead of pulling the 3.9.0 wheel. We have now yanked the whole 3.9.1 release to fix these problems and will get 3.9.2 out as quick as we can.

To prevent this sort of thing in the future (trying to build something from source when you are not prepared to do so) I suggest using matplotlib --only-binary "matplotlib" in your requirements file (which may be a good practice for all wheels that have extensions).

Sorry for the trouble.

richardotis and others added 2 commits August 6, 2024 16:39
Per PEP-3102, we can syntactically disallow any positional arguments beyond the first four

Co-authored-by: Brandon Bocklund <brandonbocklund@gmail.com>
.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch wks-init-order
# Changes to be committed:
#	modified:   pycalphad/core/workspace.py
#
# Changes not staged for commit:
#	modified:   pycalphad/core/minimizer.pyx
#
# Untracked files:
#	.eggs/
#	.pyodide-xbuildenv-0.27.0/
#	.pyodide-xbuildenv/
#	.vscode/launch.json
#	2020-12-solver-notes.txt
#	2022-04-01-Bocklund-pycalphad-todo.docx
#	4sub.txt
#	Matcalc-Fe_Co-Cr-Fe-Nb-Ti.tdb
#	Who are pycalphad users today.docx
#	activate
#	alni.prof
#	alni.py
#	alni.txt
#	conda_recipe/
#	coverage.xml
#	docs/pycalphad-logo-notext-opt-test-copy.svg
#	docs/pycalphad-logo-notext-opt-test.svg
#	docs/pycalphad-logo-notext-opt.svg
#	docs/pycalphad-logo-notext.svg
#	docs/pycalphad-logo.svg
#	eqnew.txt
#	examples-old/
#	examples/AlNi.ipynb.old
#	examples/CustomProperty.ipynb
#	examples/ElasticProperties.ipynb
#	examples/ElasticTi.tdb
#	examples/Li_O.tdb
#	examples/Ti-working database_v2_3_23_2017.tdb
#	examples/alni-espei.tdb
#	examples/databases/
#	examples/doo.py
#	feo.prof
#	feo.py
#	fun.py
#	new.prof
#	ns163-symengine.prof
#	ns163.prof
#	ns163.py
#	ns163.py.lprof
#	old.prof
#	out.prof
#	out2.prof
#	out3.prof
#	points-array-py37.npy
#	points-array.npy
#	profile_output.lprof
#	profile_output.txt
#	profile_output_2024-07-26T102754.txt
#	profile_output_2024-07-26T103518.txt
#	profile_output_2024-07-26T104024.txt
#	pycalphad.zip
#	results.txt
#	sympy-perf-19.prof
#	sympy-perf-develop.prof
#	sympy-perf.py
#	test.py
#	test.txt
#	wks-perf.prof
#	wks-perf.py
#	workspace.prof
#
@richardotis richardotis merged commit 55d5d2b into pycalphad:develop Aug 6, 2024
@richardotis richardotis deleted the wks-init-order branch August 6, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants