Skip to content

Add logging in project based on 'log' library.#12

Merged
Narsil merged 2 commits into
huggingface:mainfrom
evgenyigumnov:logging
Aug 15, 2023
Merged

Add logging in project based on 'log' library.#12
Narsil merged 2 commits into
huggingface:mainfrom
evgenyigumnov:logging

Conversation

@evgenyigumnov

Copy link
Copy Markdown
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"
Comment thread Cargo.toml Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated

@Narsil Narsil left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/lib.rs Outdated
@McPatate

Copy link
Copy Markdown
Member

Can we use tracing instead of env_logger ?

@evgenyigumnov

evgenyigumnov commented Aug 15, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@evgenyigumnov

Copy link
Copy Markdown
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.

@Narsil Narsil left a comment

Copy link
Copy Markdown
Contributor

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

Narsil commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Narsil commented Aug 17, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Narsil commented Aug 18, 2023

Copy link
Copy Markdown
Contributor

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