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

PERF: delay preset loading for hutch-python #394

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Jan 14, 2025

Description

Motivation and Context

We want to defer preset loading for performance reasons.

This will fail tests until the corresponding pcdsdevices PR is merged.

How Has This Been Tested?

I was using this to test presets while developing pcdshub/pcdsdevices#1317

Where Has This Been Documented?

This PR

Left: Before, Right: After
image

In getting that screenshot I ran into a variety of load times, preset deferral was always better but by varying margins.

Ran on a clone of XCS's hutch-python profile, copied into my dev area

Pre-merge checklist

  • Code works interactively (a real hutch config file can be loaded)
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong force-pushed the perf_defer_preset_loading branch from 90915dc to 8c22e1a Compare January 15, 2025 01:27
@tangkong tangkong marked this pull request as ready for review January 15, 2025 17:14
@tangkong
Copy link
Contributor Author

I guess there are no real tests to do here, since this is a pcdsdevices feature.

@ZLLentz
Copy link
Member

ZLLentz commented Jan 15, 2025

Does this implicitly change our minimum supported pcdsdevices version? Is this something we would address in the conda forge feedstock?

@tangkong
Copy link
Contributor Author

tangkong commented Jan 15, 2025

Ah you're right. That slipped my mind.

This will technically rely on a pcdsdevices version that doesn't exist yet. I'm reluctant to tag pcdsdevices right now because we might accumulate more changes, but once I change the pins the tests will fail. I'll move this back to WIP until we're ready to more forward.

More incentive for me to look at pyca / typhos I guess

@tangkong tangkong marked this pull request as draft January 15, 2025 17:37
@ZLLentz
Copy link
Member

ZLLentz commented Feb 14, 2025

I found this again in my PR reviews backlog. Maybe we should tag pcdsdevices next week.

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.

2 participants