Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing docstrings for Base.Sys #52777

Merged
merged 36 commits into from
Jan 19, 2024
Merged

Conversation

11happy
Copy link
Contributor

@11happy 11happy commented Jan 6, 2024

Part of #52725 .
Thank you.

CC:

@inkydragon inkydragon added the docs This change adds or pertains to documentation label Jan 6, 2024
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
@stevengj stevengj changed the title Added Docstring for Base.sys Added Docstring for Base.Sys Jan 6, 2024
@stevengj stevengj changed the title Added Docstring for Base.Sys add missing docstrings for Base.Sys Jan 6, 2024
@11happy 11happy requested review from stevengj and inkydragon January 7, 2024 12:27
Signed-off-by: 11happy <[email protected]>
@11happy
Copy link
Contributor Author

11happy commented Jan 8, 2024

Hello @stevengj & @inkydragon I have incorporated the suggested changes can you please look into it.
Thanks

base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
base/sysinfo.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

stevengj commented Jan 8, 2024

It seems like a historical artifact that we have both cpu_info() to get a Vector{CPUinfo} and cpu_summary() which gets a nice way to print it. I think this is because the function pre-dates the 3-argument show functions for pretty-printing at the REPL. We should really clean this up before we add anything to the manual.

It seems like we should just add a show method that displays a Vector{CPUinfo} via the easier-to-read cpu_summary() function, so users would never need to call cpu_summary() directly to get a nicer tabulated format:

function show(io::IO, ::MIME"text/plain", cpu::AbstractVector{CPUinfo})
    summary(io, cpu)
    println(io, ':')
    cpu_summary(io, cpu)
end

We can continue to export cpu_summary for backwards compatibility, but we shouldn't put it in the manual since there will be no reason to call it directly anymore.

Also, the show(io::IO, info::CPUinfo) method should really be a show(io::IO, ::MIME"text/plain", info::CPUinfo) method for pretty-printing — we should use the default 2-argument show (used in compact display like print) so that round-tripping with repr works, and the pretty-printed display should be a 3-argument show call (used e.g. by the REPL display function). Again, the current behavior is because this function pre-dated the 3-argument show.

And if we are documenting the CPUinfo struct, it should really be declared public and get its own docstring.

11happy and others added 3 commits January 8, 2024 18:20
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
base/sysinfo.jl Outdated Show resolved Hide resolved
Co-authored-by: Steven G. Johnson <[email protected]>
11happy and others added 4 commits January 16, 2024 19:54
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
11happy and others added 2 commits January 16, 2024 21:38
Co-authored-by: Steven G. Johnson <[email protected]>
Signed-off-by: 11happy <[email protected]>
@stevengj stevengj added the merge me PR is reviewed. Merge when all tests are passing label Jan 16, 2024
test/sysinfo.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

Unrelated CI failures, should be good to merge.

inkydragon added a commit that referenced this pull request Jan 19, 2024
I noticed in #52777 that `Sys.SC_CLK_TCK` is set to `0` on Windows,
which is treated as an unknown clock-tick unit.

This undocumented global variable is *only* used by `Sys` to display CPU
load times from the `CPUInfo` data structure, which until #52777 was
completely undocumented and only seems to be used by
`versioninfo(verbose=true)`. On POSIX systems it is set to a `> 0`
value, and is used to convert the CPU times into seconds. On Windows, it
is set to `0`, which is treated as an unknown unit, so the CPU times are
displayed without units.

Since it is only used for CPU times, we can look at the [`uv_cpu_info`
implementation](https://github.com/libuv/libuv/blob/3b6a1a14caeeeaf5510f2939a8e28ed9ba0ad968/src/win/util.c#L531-L653)
to see how libuv computes these values on Windows. This function calls
`pNtQuerySystemInformation`, which returns times in ["100ns
intervals"](https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntquerysysteminformation#system_processor_performance_information),
i.e. in units of 10⁻⁷ seconds. However, `uv_cpu_info` then divides these
values by `10000`, which converts them to units of milliseconds.

So, I believe that the correct value of `Sys.SC_CLK_TCK` on Windows is
`1000`.

Basically this just means that `versioninfo(verbose=true)` output on
Windows will get units of seconds when printing CPU loads. As far as I
can tell, no external package is using these (undocumented until now)
timing values — the few packages that call `Sys.cpu_info()` only do so
to get the `model` (string) field.
@inkydragon inkydragon merged commit eb26d63 into JuliaLang:master Jan 19, 2024
5 of 7 checks passed
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Jan 19, 2024
@nsajko
Copy link
Contributor

nsajko commented Jan 19, 2024

Ten different "update sysinfo.jl" commits being in git log is spammy.

@11happy
Copy link
Contributor Author

11happy commented Jan 19, 2024

Ten different "update sysinfo.jl" commits being in git log is spammy.

Those are actually the commit suggestions directly using Github UI as suggested by @stevengj .

@11happy
Copy link
Contributor Author

11happy commented Jan 19, 2024

I am sorry for such messy commit history,I will keep this in mind next time.

@nsajko
Copy link
Contributor

nsajko commented Jan 19, 2024

It's not necessarily your fault (and it's not some huge issue anyway). I suppose @inkydragon should have squashed the commits while/before merging into master.

@stevengj
Copy link
Member

I wish there was a way to disable anything but "squash and merge" on github for the whole repository. It's too easy to click "merge" instead by accident.

@stevengj
Copy link
Member

It looks like it was squashed into a single commit eb26d63, so what's the problem?

@nsajko
Copy link
Contributor

nsajko commented Jan 19, 2024

All the commits are now on the master branch, I see them cluttering my git log. Well, now I'm cluttering this PR's comments, but I always appreciated a tidy git history, as the commits on master are supposed to be somewhat permanent. For example, I see all of these in my git log for master:

d4e3a3d

adbf6b0

718740e

6cf7d26

06f5a4c

5e2cdbd

3309d86

Not a big deal in any case.

@stevengj
Copy link
Member

Oh, I see, I was just looking at the merge commit. Looks like the same thing (merge, not squash-and-merge) happened with #52936 😢

@inkydragon
Copy link
Member

Apologies for the mess, I didn't realize that the default merge method should be squash-and-merge.

Not sure if there is anything I can do to eliminate the mess.
From what I know of git, clearing existing commits requires git push --force, which is a dangerous
operation, and I'm not prepared to do it.

On the other hand, the default merge method is a convention that should probably be documented in devdoc.
I'm going to open a new pr later to document this convention.

And furthermore, Julia needs something like a Python developer's guide.
https://devguide.python.org/core-developers/committing/

@nsajko
Copy link
Contributor

nsajko commented Jan 19, 2024

clearing existing commits requires git push --force, which is a dangerous operation, and I'm not prepared to do it.

Definitely don't do that, rewriting the history for master would cause real breakage. Although I know the Golang people actually did that at least once, I think because someone accidentally committed a big binary file or something.

On the other hand, the default merge method is a convention that should probably be documented in devdoc.
I'm going to open a new pr later to document this convention.

From what I've observed squash-and-merge is what people do here unless there's a label:"don't squash" label on the PR. That said, TBH it should be common sense not to put 30 redundant commits into the Git history.

@stevengj
Copy link
Member

stevengj commented Jan 20, 2024

Besides simply reducing clutter, squashing is important because otherwise you will often get commits in the history that leave the repo in a broken state, breaking git bisect.

I agree that it should only be done retro-actively in drastic situations, and that it should be added to developer docs.

But I think that this is mainly a UI issue — it's just too easy to click the "merge" button accidentally, when in fact this should be marked as a dangerous action. Apparently there is a way to require squash-and-merge for a branch on github, so maybe we should do that: https://github.com/orgs/community/discussions/50593#discussioncomment-5402508 whoops, I should have read farther, apparently these instructions are a hallucination.

@stevengj
Copy link
Member

I filed an issue about squash-merging (see link above) — let's continue further discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants