-
Notifications
You must be signed in to change notification settings - Fork 0
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
DM-48471 : Create Release 0.2.0 #159
base: main
Are you sure you want to change the base?
Conversation
8eda6b5
to
d5f142c
Compare
aca3cf1
to
4ff6c31
Compare
88bfbf0
to
a6511af
Compare
- Update uv version in bootstrap script - Update dependencies - Remove pause package dependency - include libexpat1 in base image - cleanup makefile, readme, comments - Remove usdf db-altering targets from makefile - Remove unneeded pylint directives - Update README. Relocate script. - Correct default ASGI__PREFIX setting. - Update ruff version in pre-commit - Update gitignore - Update default htcondor host in script template - Extend settings models - update ruff formatting, remove pause, fixup actions
a6511af
to
a58184e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright to me.
src/lsst/cmservice/common/butler.py
Outdated
repo_uri = repo | ||
|
||
try: | ||
bc = ButlerConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instantiating a ButlerConfig
does synchronous I/O, so you probably don't want to do this in an async function without using a threadpool.
(You should assume that basically every function/constructor/method in daf_butler
, resources
, and other adjacent packages does synchronous I/O -- even if it seems like it shouldn't need to. I have been caught by surprise many times.)
@@ -220,9 +221,10 @@ async def split( | |||
if mock_butler: | |||
sorted_field_values = np.arange(10) | |||
else: | |||
butler_config = await get_butler_config(butler_repo, without_datastore=True) | |||
butler_f = partial( | |||
Butler.from_config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI instantiating a Butler is rather expensive (typically 1-2 seconds snuffling around configuration files and the database at startup, and you start with empty caches for other database-snufflings that happen when you call methods later.)
If this function doesn't get called very often that's probably fine, but you might consider some other ways of instantiating a Butler:
- Use LabeledButlerFactory to instantiate the Butler for each request -- this shares a database connection pool and various caches between instances so they start up faster.
- Keep a 'template' Butler instance , and use
Butler.clone()
to make a copy of it for each request. - Keep a pool of Butler instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a new Jira story (DM-48690) to address these issues and implement an effective butler pool, as I think it will be more important as I expand the amount of in-process Butler operations but for right now these are few and far between.
src/lsst/cmservice/config.py
Outdated
access_token: str | None = Field( | ||
description=("Gafaelfawr access token used to authenticate to a Butler server."), | ||
default=None, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need access_token
, and if you did it would come in via an HTTP header instead of this configuration.
It only applies to services running on the Rubin Science Platform, using Gafaelfawr authentication to access Butler server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because I saw the token in the signature for the create_butler
method of the LabeledButlerFactory, thinking it might be useful to have ready to go, but if there's no future case it'd be necessary I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for your service I don't anticipate you'll ever need it.
fix: Allow alias paths in condor submit files
b8fded6
to
7e95e3e
Compare
7e95e3e
to
0eaceab
Compare
This PR ties up some loose ends and creates the 0.2.0 release version.