Conversation
Nothing in the repo published to or consumed from the exchanges and queues that RabbitAdaptor declared, so every gunicorn worker kept a blocking connection open for queues that only accumulated in the broker. This drops the adaptor, the _setup_rabbit_queues plumbing threaded through create_app, add_routes and SubmitTransformationRequest.make_api, and the now unused pika dependency; real transform work continues to flow through Celery, which keeps using RABBIT_MQ_URL as its broker. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
The app factory carried a create_tables() helper nobody called, an unreachable return in StreamFormatter.format, a sys.path.append for a celery directory the app pod does not mount, and a lambda wrapper around a plain call in _override_config_with_environ. decorators.py had a no-op assert on a string literal and an except clause that paired NoAuthorizationError with the Exception that already subsumes it. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
… (S5) The try/except NoResultFound wrappers around .all() queries could never fire, since .all() returns an empty list rather than raising, and Dataset get_by_did_finder and get_all returned a bare query in some branches and a list in others despite being annotated as lists. Both now call .all() consistently, a commented-out legacy lookup is gone, and migrations/env.py drops the Flask-SQLAlchemy < 3 TypeError fallback for a project that requires version 3. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
add_files_to_processing_queue rejected work based on an is_complete check against a stale JSON snapshot, duplicating the live check that LookupResultProcessor already performs. The two ways of building the advertised app URL are now one function that prefers ADVERTISED_HOSTNAME from the Flask config and falls back to the instance name for celery workers, which run outside an app context, and task routing moves from a celeryd_after_setup hook to a task_routes declaration in celeryconfig. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
The admin index checked isinstance(view, ReportView) twice on the same view, and the "ServiceX Reports" admin exposed its index without the auth mixin the main admin uses, so it now gets a secured index view with tests covering both the admin and the anonymous case. The results resource calls the to_json_list classmethod instead of passing an instance to to_json, and drops a missing request_id branch the route cannot reach. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
distutils was removed from the standard library in Python 3.12, so importing strtobool from it blocks the upgrade. A small private helper accepts the same true and false spellings, case insensitively, and raises ValueError on anything else. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
Dataset.find_by_id used the legacy Query.get API, which emits a LegacyAPIWarning under SQLAlchemy 2. It now uses db.session.get, matching UserModel.find_by_id, and its return annotation uses a PEP 604 union. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
check_user_exists was a one-line alias for UserModel.find_by_sub, so callers now use the model directly. add_user rolls back and re-raises instead of printing the exception and reporting success, and list_users builds a single query rather than fetching every user and then running a second filtered query. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
servicex_app/.pre-commit-config.yaml pinned flake8 3.8.4 from a gitlab URL that can no longer be cloned, which broke a repo-wide pre-commit run. The root config is the one CI uses. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
ariostas
force-pushed
the
refactor/app-cleanup
branch
from
September 9, 2026 14:00
cb40ff1 to
cd942b4
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Removes the vestigial RabbitMQ adaptor from the Flask app and clears out a batch of dead code, duplicated logic and legacy API calls that the repo review turned up. No behavior change is intended beyond two noted cases: the "ServiceX Reports" admin index is now access controlled, and the CLI user create command no longer swallows database errors. A few signatures that were already being edited also picked up PEP 604 unions (M5); there was no repo-wide sweep for that, so it does not get a commit of its own.
How to review. Each finding is one commit; the Commits tab shows them in severity order. Each entry below links to its commit; tick approve or reject under it. To ask for a change instead, leave a review comment on the commit. Rejected commits will be dropped from the branch and this list will be updated to match.
2e57dd0· Deleterabbit_adaptor.pyand therabbitmq_adaptorplumbing throughcreate_app,add_routesandmake_api, plus thepikadependency; nothing published to or consumed from those exchanges and queues, and real transform work flows through Celery, which keepsRABBIT_MQ_URLas its broker.53202e2· Drop the uncalledcreate_tables(), the unreachablereturn super().format(record), thesys.path.append("/opt/servicex/celery")for a directory the app pod does not mount, and the lambda wrapper in_override_config_with_environ; indecorators.pydrop the no-opassert "NoAuthorizationError"and collapseexcept (NoAuthorizationError, Exception).53f0d6a· Remove thetry/except NoResultFoundwrappers around.all()queries, which cannot fire, makeDataset.get_by_did_finderandDataset.get_allreturn.all()in every branch so they match their list annotations, drop a commented-out lookup, and remove the Flask-SQLAlchemy < 3 fallback inmigrations/env.py.3e7121d· Drop theis_completerejection inadd_files_to_processing_queue, which duplicated the live check inLookupResultProcessoroff a stale JSON snapshot, unifyadvertised_endpoint()withServiceXResource._generate_advertised_endpoint, and move task routing from aceleryd_after_setuphook totask_routesinceleryconfig.py.ee5ea9d· Remove the redundant secondisinstance(view, ReportView)check and give the "ServiceX Reports" admin a secured index view mirroring the main admin; in the results resource use theto_json_listclassmethod instead of callingto_jsonon an instance and drop an unreachablerequest_idbranch.8d8d38c· Replacedistutils.util.strtobool, removed from the standard library in Python 3.12, with a small private helper accepting the same true and false spellings case insensitively.d46cda5·Dataset.find_by_idusesdb.session.get(cls, id)instead of the legacycls.query.get(id), matchingUserModel.find_by_idand silencing theLegacyAPIWarning.9da66f2· Inlinecheck_user_exists, a one-line alias forUserModel.find_by_sub;add_userrolls back and re-raises instead of printing the exception and reporting success;list_usersbuilds one query instead of fetching every user and then running a second filtered query.cd942b4· Deleteservicex_app/.pre-commit-config.yaml, which pinned flake8 3.8.4 from a gitlab URL that can no longer be cloned and so broke a repo-widepre-commit run -a. The root config is the one CI uses.Skipped
resources/datasets/get_all.py: the"show-deleted" in argscheck always being true was left alone to avoid colliding with other in-flight work on the resource handlers.transformer_manager.pyare out of this PR's scope.cancel_consumerintransformer_manager.pyis a Celery control call, not part of the removed adaptor, so it was left alone.Follow-ups noticed, out of scope
servicex_app/.flake8excludes three files that no longer exist:validate_requests.py,find_m.py,messaging.py.servicex_app/pyproject.tomlstill listspre-commit = "^2.20.0"in the dev group.Part of #1539.