Skip to content

Conversation

@cnweaver
Copy link
Contributor

@cnweaver cnweaver commented Sep 26, 2025

Description

This attempts to fix #248 while keeping large message offload support working by default.

Checklist

  • All new functions and classes are documented and adhere to Google doc style (3.8.3-3.8.6 of this document)
  • Add/update sphinx documentation with any relevant changes.
  • Add/update pytest-style tests in /tests, ensuring sufficient code coverage.
  • make test runs without errors.
  • make lint doesn't give any warnings.
  • make format doesn't give any code formatting suggestions.
  • make doc runs without errors and generated docs render correctly.
  • Check that CI pipeline run on this PR passes all stages.
  • Review signoff by at least one developer.

NOTE: If this PR relates to a release, open and reference an issue with the Release checklist template.

@cnweaver cnweaver self-assigned this Sep 26, 2025
@cnweaver cnweaver added the bug Something isn't working label Sep 26, 2025
@cnweaver
Copy link
Contributor Author

Tests generally work, but the linter is failing.
Making sure that all licensing terms are met (particularly when packaging) still needs to be done.

@codecov
Copy link

codecov bot commented Sep 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.76%. Comparing base (98590a4) to head (51023d6).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #250   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files          16       16           
  Lines        2126     2126           
=======================================
  Hits         2121     2121           
  Misses          5        5           
Flag Coverage Δ
pytest 99.76% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cnweaver cnweaver marked this pull request as ready for review September 29, 2025 20:58
Copy link

@jnation3406 jnation3406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not up to speed on what the core issue was, but it seems like pymongo packages a bson implementation within it and installs it separately as bson, but its really a modified version of bson and not what you have copied here and wanted installed? Is the main problem that users of hop client that already have mongodb installed break their pymongo when installing hop-client since it installs the base bson over the already installed customized mongodb bson? It looks like its using a c implementation for a some of the bson stuff in pymongo here... Would it work to just depend on pymongo (which contains bson) rather than bson in the hop-client? It seems like the internal pymongo bson codec might be more efficient anyway since it uses c.

@cnweaver
Copy link
Contributor Author

#248 describes the original problem pretty succintly, which is that if hop-client is installed (it need not even be imported), pymongo is broken and cannot be imported.
As best I can tell the 'bson' installed by pymongo is probably the original in the python space, but it is handled in an awkward way (which is the cause of our problem), and it is as you note a C extension, which requires per-target wheels. The bson package, copied here, is a lighter-weight, pure python implementation which may not be as performant, but has been adequate for our needs (which are pretty minimal), and keeps our code architecture-independent.
I think we don't really want to add a pymongo dependency since we 1) don't really use it and 2) it makes a mess that I'd rather not spread that to affect more people who don't expect it. It would have been nice to optionally use the pymongo version if it is already installed, but pyproject.toml is, as far as I can tell (and not unreasably), not expressive enough to make a dependency conditional on other packages not being present in the installation environment.

@jvansanten jvansanten mentioned this pull request Sep 30, 2025
9 tasks
Copy link
Contributor

@adambrazier adambrazier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (inasmuch as I understand pyproject.toml)

@cnweaver cnweaver merged commit f728059 into master Oct 2, 2025
8 checks passed
jvansanten added a commit to AmpelAstro/Ampel-HU-astro that referenced this pull request Oct 23, 2025
jvansanten added a commit to AmpelAstro/Ampel-HU-astro that referenced this pull request Dec 9, 2025
* Activate LSPhotoZ with Secrets + PlotLightcurve with new Slack API.

* Jobs and processes to try out LSST units.

* mypy fixes

* Set avro schema for elasticc alerts

* Bump version

* Use plot API for parsnip plots

* Begin typing BaseClassifier internals

* Relax typing of plot

* First crack at LSSTReport

* Bail on empty or missing T2RunParsnipRiseDecline result

* register T2LSSTReport

* chore: Update README for 7c6c243

* Fix NamedSecret bug.

* New Decent filter for VRO.

* Units for filtering+specific reporting of VRO alerts.

* chore: Update README for 7c6c243

* chore: format

* fixup! Units for filtering+specific reporting of VRO alerts.

Revert presumably botched merge of T2BaseClassifier

* fixup! Begin typing BaseClassifier internals

* Remove report_t2s; information already present in t2_dependency

* Avoid possibly unbound features

* fixup! Remove report_t2s; information already present in t2_dependency

* Annotate fields with docs

* use AvroSchema in KafkaAdapter

* Support general auth schemes

* Add Hopskotch adapter

* Include compound id in report

* simplify torch requirement

* drop implicit bson requirement

See scimma/hop-client#250

* Use ampel secret.

* More general reports.

* Bugfixes for report units.

* Adding source report field.

* sample lsst jobs

* Check xaxis length, and do not do slackpost if nothing found.

* Sample job for filtering and plotting vro alerts.

* mypy fixes

* Remove old parsnip plot commans.

* Adding T2 unit for Lasair annotations.

* Updated config and demos for lasair annotation.

* change name

* Inherit from AbsTabulatedT2Unit only once

* advance ampel-lsst

* ruff

* advance ampel-lsst again

* advance ampel-ztf (astropy 7.2 compat)

* shiny for mypy

* Drop built-in channel and process defs

* ignore import errors on unit inventory

* Drop py 3.11 support, too

* chore: Update README for 8d425e5

* refresh lock to pick up latest ampel-core

that hopefully allows configs to build without error out of the box

* make lasair optional

* remove boilerplate

* avoid post_init antipattern

* avoid post_init antipattern

---------

Co-authored-by: Jakob van Santen <jvansanten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hop-client conflicts with pymongo

4 participants