Skip to content

Conversation

@evgenyigumnov
Copy link
Contributor

Add stdout logs in tests.

Example stdout logging during tests:

[2023-08-12T08:45:02Z WARN hf_hub] File with your token is absent by path "C:\Users\igumn\.cache\huggingface\token"

Add stdout logs in tests.

Example stdout logging during tests:

[2023-08-12T08:45:02Z WARN  hf_hub] File with your token is absent by path "C:\\Users\\igumn\\.cache\\huggingface\\token"
Copy link
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Happy to improve a bit the logging, but left a few remarks regarding style.
Happy to make the modifications myself.

@McPatate
Copy link
Member

Can we use tracing instead of env_logger ?

@evgenyigumnov
Copy link
Contributor Author

evgenyigumnov commented Aug 15, 2023

Can we use tracing instead of env_logger ?

Yes, there are list of loggers:

simple_logger
simplelog
pretty_env_logger
stderrlog
flexi_logger
call_logger
std-logger
structured-logger
log4rs
fern

You could visit page https://crates.io/crates/log and find all list of loggers

@McPatate
Copy link
Member

I was talking about the https://lib.rs/crates/tracing crate specifically :)

@evgenyigumnov
Copy link
Contributor Author

I was talking about the https://lib.rs/crates/tracing crate specifically :)

If you ask my opinion. I think, yes it is possible to use tracing libray for logging like log library.
But what I see:

  1. I try to find tracing library using in project code. It is not look like simple logging
  2. I never use tracing library in my project. If project manager of "hf-hub" make decision use tracing for logging. Then I could learn tracing library and change my pull request from log to tracing library.

Copy link
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks for the modifications.

@Narsil
Copy link
Contributor

Narsil commented Aug 15, 2023

I'll go ahead and merge for now, we can revisit later.

IMO this crate should be minimalistic, hence logging (and sparingly) should be OK.
Users can always trace themselves to the actual potential bottlenecks like get and download.
All other functions should be rather trivial.

@Narsil Narsil merged commit bad4339 into huggingface:main Aug 15, 2023
@McPatate
Copy link
Member

For the record, I'm not happy about this 😄

tracing is a standard and used pretty much everywhere for logging. I think we should revisit eventually!

@Narsil
Copy link
Contributor

Narsil commented Aug 17, 2023

https://crates.io/crates/tracing (~200k/day)
https://crates.io/crates/log (~400k/day)

I don't care that much about it either way, ideally I think we just should just have neither, or behind feature flags.

@McPatate
Copy link
Member

Ah I didn't think log was as ubiquitous, looks like a sync vs async divide, tokio has a dependency on tracing while most sync libraries have log.

I also thought log had less features for libraries, but either I missed it at the time, either it caught up.

@Narsil
Copy link
Contributor

Narsil commented Aug 18, 2023

You live in async world ! :)

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.

3 participants