Skip to content

Conversation

@cianc
Copy link
Contributor

@cianc cianc commented Nov 27, 2025

Description

Add unit tests for logfire and http output methods.
This requires adding logfire as a dev dependency so we can import it in tests.
Also make it clearer which EmissionsData params are unnecessary for the out and live_out methods.

Related Issue

Please link to the issue this PR resolves: #972

Motivation and Context

Increases test coverage.

How Has This Been Tested?

Ran all existing and new tests.

Screenshots (if appropriate):

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@cianc cianc marked this pull request as ready for review November 27, 2025 19:39
@cianc cianc requested a review from a team as a code owner November 27, 2025 19:39
@benoit-cty benoit-cty closed this Nov 29, 2025
@benoit-cty benoit-cty force-pushed the test/http_logfire_output branch from e8dc95e to d0ff5e4 Compare November 29, 2025 08:55
@benoit-cty
Copy link
Contributor

@cianc , can you confirm that every changes was merged by #989 ?
Or did I do something wrong ?

@cianc
Copy link
Contributor Author

cianc commented Nov 29, 2025

Not sure what happened here, maybe I messed pu my branches.
Looks like https://github.com/mlco2/codecarbon/pull/989/files contained all the files from this PR.

@cianc
Copy link
Contributor Author

cianc commented Nov 29, 2025

One thing: I see this failure https://github.com/mlco2/codecarbon/actions/runs/19781662666/job/56682713399 complaining:

E   ModuleNotFoundError: No module named 'logfire'

This works in my local environment

cianc@fedora:~/src/codecarbon$ .venv/bin/python3
Python 3.14.0 (main, Oct 17 2025, 00:00:00) [GCC 15.2.1 20251022 (Red Hat 15.2.1-3)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import logfire
>>> logfire.__doc__
'**Logfire** is the observability tool focused on developer experience.'

I did add logfire to to dev dependencies in pyproject.toml, but maybe that wasn't enough?

@inimaz
Copy link
Collaborator

inimaz commented Nov 29, 2025

Interesting issue!
Digging a bit, here there is a summary:

  • The test passed in the branch but failed in master.
  • This is because in the master script we install codecarbon directly from the wheel so it does not have all the optional packages. We did this to test how the package would behave when the user installs it via pypi/conda.

TLDR: the test should fail in master as expected because there is no logfire unless the user installs it😄 Thanks @cianc for spotting this!

Solutions:

  1. We can maybe edit the test to try to import logfire and if not present in the python modules skip the test?
  2. We can flag what tests to not run when running directly the wheel
  3. Any other ideas?

There is a similar issue with prometheus for instance and any other output_method that needs an external library. In the prometheus case we imported it into the codecarbon package but in retrospective I do not think it is the best way. It does not scale, if we implement 30 ouput_methods, we will not want the 30 libraries inside codecarbon when each end user just needs one or two.

@cianc
Copy link
Contributor Author

cianc commented Nov 29, 2025

Another option: can we add logfire to this line where we're already installing pandas?
https://github.com/mlco2/codecarbon/blame/d0ff5e47eb5c7576774dfa38d097ae0cf78a7b25/.github/workflows/package.yml#L62

@inimaz inimaz mentioned this pull request Nov 29, 2025
9 tasks
@cianc cianc deleted the test/http_logfire_output branch November 29, 2025 15:39
@benoit-cty
Copy link
Contributor

There is a similar issue with prometheus for instance and any other output_method that needs an external library. In the prometheus case we imported it into the codecarbon package but in retrospective I do not think it is the best way. It does not scale, if we implement 30 ouput_methods, we will not want the 30 libraries inside codecarbon when each end user just needs one or two.

I totally agree : we need to have minimal dependencies for default install (reduce dependency hell and lower attack surface) and provide optional dependencies for output method.
And implement try..except in modules.

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.

[Tests] Add unit tests to the output methods

4 participants