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 support for Cthulhu #101

Merged
merged 8 commits into from
Jun 30, 2020
Merged

Add support for Cthulhu #101

merged 8 commits into from
Jun 30, 2020

Conversation

timholy
Copy link
Owner

@timholy timholy commented Jun 23, 2020

This is the final tool (so far) for fixing invalidations. ascend makes it much easier to identify a fruitful place to intervene and fix inference problems.

This also adds tools for bumping the versions across-the-board. What do you think of this, @aminya? Overall I think our best strategy is to keep all version numbers in lock-step, and that's the workflow made easy with these tools. When you want to bump the version, you say bump_version(v"1.7.0"). After the PR merges, update your local master branch to the latest, check out a branch of registries/General, and then register_all(). Then submit your registries/General as a PR.

@timholy
Copy link
Owner Author

timholy commented Jun 23, 2020

Argh, we're being bitten by JuliaLang/Pkg.jl#1874. Naturally. I'd forgotten about that for a moment.

Have to switch to other things, will consider a fix soon.

@aminya
Copy link
Contributor

aminya commented Jun 23, 2020

Thanks for this! I think this bump_version should eventually go to Pkg. I will take a more thorough look soon.

@KristofferC
Copy link
Collaborator

KristofferC commented Jun 23, 2020

Thanks for this! I think this bump_version should eventually go to Pkg.

Probably better in RegistryTools or something similar. Remember that Pkg has the disadvantage of being a stdlib and is thus slow to make fixes for and should not ideally break it's API etc. Seems much better for something like this somewhere without these constraints.

@timholy
Copy link
Owner Author

timholy commented Jun 23, 2020

Thanks @KristofferC for suggesting the manifest.

@aminya, I switched the doc-build to nightly, since only nightly has the capabilities for the invalidations. I also fixed a couple of issues with the bot docs that emerged due to the split. Unfortunately we have two broken hyperlinks, from @snoopr to invalidation_trees and back, due to the fact that neither module knows about the other. I don't see a good fix for that. It's only an issue in the reference section.

@aminya

This comment has been minimized.

@@ -11,6 +11,9 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: julia-actions/setup-julia@latest
with:
version: nightly
Copy link
Contributor

@aminya aminya Jun 23, 2020

Choose a reason for hiding this comment

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

Adding this to both Documenter.yml and ci.yml can solve the downloading issues partially.

Suggested change
version: nightly
version: nightly
- name: Cache artifacts
uses: actions/cache@v1
env:
cache-name: cache-artifacts
with:
path: ~/.julia/artifacts
key: ${{ runner.os }}-test-${{ env.cache-name }}-${{ hashFiles('**/Project.toml') }}
restore-keys: |
${{ runner.os }}-test-${{ env.cache-name }}-
${{ runner.os }}-test-
${{ runner.os }}-

@aminya
Copy link
Contributor

aminya commented Jun 23, 2020

Unfortunately we have two broken hyperlinks, from @snoopr to invalidation_trees and back, due to the fact that neither module knows about the other. I don't see a good fix for that. It's only an issue in the reference section.

This is this issue. I suggested a possible solution some time back, but I got no reaction.
JuliaDocs/Documenter.jl#688

We can use full links instead of [](@ref), to solve that temporarily

@timholy
Copy link
Owner Author

timholy commented Jun 23, 2020

Cthulthu 1.2.0 is not installed automatically. Is it registered?

Yep, just update your packages. https://github.com/JuliaDebug/Cthulhu.jl/releases/tag/v1.2.0

@aminya

This comment has been minimized.

@aminya
Copy link
Contributor

aminya commented Jun 23, 2020

OK, I can finally pass the tests locally.

It might be an issue from my end. I fail to update my registry.

(@v1.6) pkg> up
   Updating registry at `C:\Users\yahyaaba\.julia\registries\General`
┌ Warning: Some registries failed to update:
│     — `C:\Users\yahyaaba\.julia\registries\General` — registry dirty
└ @ Pkg.Types D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.6\Pkg\src\Types.jl:1194
No Changes to `C:\Users\yahyaaba\.julia\environments\v1.6\Project.toml` 
No Changes to `C:\Users\yahyaaba\.julia\environments\v1.6\Manifest.toml`

I had to delete my julia/registries to fix this.

@@ -4,6 +4,7 @@ export BotConfig, snoop_bot, snoop_bench

using Core: MethodInstance, CodeInfo
using YAML
using SnoopCompileCore # needed for the [`@snoopi`](@ref) doc links
Copy link
Contributor

@aminya aminya Jun 23, 2020

Choose a reason for hiding this comment

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

Do we really need to do this? 🤔 Now I use external processes for snooping, and so I don't use SnoopCompileCore directly. So, I prefer to use full links instead of loading the package. It does not seem a good approach to load the packages just for the sake of cross-links.

Suggested change
using SnoopCompileCore # needed for the [`@snoopi`](@ref) doc links

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see your point, given that you do everything in external processes this is not ideal and should probably be deleted. But the manual cross-link is not as robust. Suppose someone makes some local changes and builds the docs locally. Then won't cross-links refer, not to their local copy of the documentation, but to the global one? That's not good.

@aminya
Copy link
Contributor

aminya commented Jun 24, 2020

I made a simple fix for cross-linking in Documenter. Let's see if it gets approved.
https://github.com/JuliaDocs/Documenter.jl/pull/1351/files

Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

@timholy Are we adding this as a patch or a minor?

If this is going to be a minor, could we fix #105 in a patch before merging this?

@timholy
Copy link
Owner Author

timholy commented Jun 24, 2020

This has now been rebased.

Long-term we definitely don't want the utils/register.jl, but in the short term I think let's live with it. The API is not yet general enough for RegistryTools.jl.

If you need to add your dev script then perhaps utils/ would be a better place.

@aminya
Copy link
Contributor

aminya commented Jun 24, 2020

Could we merge #103 and #109 to this PR as well?

@aminya
Copy link
Contributor

aminya commented Jun 24, 2020

I could not rebase, the conflicts were too much. I made a new PR to this branch:
#110

@timholy
Copy link
Owner Author

timholy commented Jun 25, 2020

I'm going to hold off on this until we get 1.6.2 out which fixes the bot.

timholy and others added 6 commits June 30, 2020 02:59
Co-Authored-By: Kristoffer Carlsson <kcarlsson89@gmail.com>
This is a sufficiently common operation it makes sense to export it.
Its ability to diagonse ambiguity is often wrong and will soon be
superseded by not reporting cases of ambiguity.
@timholy
Copy link
Owner Author

timholy commented Jun 30, 2020

Given #111 (comment) I think it means the bot is working again. Shall we merge this before or after fixing the conflicts in #110? Either is fine by me.

@aminya
Copy link
Contributor

aminya commented Jun 30, 2020

@timholy I have fixed the conflicts a couple of times so far. But they sneak in again.😉😄

@timholy timholy merged commit 8b496f3 into master Jun 30, 2020
@timholy timholy deleted the teh/cthulhu branch June 30, 2020 23:10
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