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

[stdlib] [benchmarks] [NFC] Fix benchmarking for public repo #4054

Closed
wants to merge 3 commits into from

Conversation

martinvuyk
Copy link
Contributor

Fix benchmarking for public repo. This uses a dirty trick of replacing magic's lib/mojo/stdlib.mojopkg because otherwise setting MODULAR_MOJO_NIGHTLY_IMPORT_PATH to the build directory doesn't actually work.

I tried using mojo run --disable-builtins -I ${BUILD_DIR} to make it import stdlib from there but it doesn't work.

Just running the script (lit.cfg.py setting MODULAR_MOJO_NIGHTLY_IMPORT_PATH) used to work before the switch to magic.

@JoeLoser JoeLoser requested a review from patrickdoc March 6, 2025 06:48
@JoeLoser
Copy link
Collaborator

@Ahajha is this PR still needed? You recently removed these nightly variables.

@Ahajha
Copy link
Collaborator

Ahajha commented Mar 10, 2025

My guess is no, but I'd like to understand the issue. I assume at time of writing, magic run benchmarks would use the packaged stdlib instead of the in-tree one?

A while back (December-ish) the nightly config sections were merged into the normal config sections (so no more distinction between mojo-max-nightly and mojo-max), and these environment variables are there to override the values. So essentially these env vars were just going into the void since that change. Though you also mentioned that the errors have existed longer than that.

@martinvuyk
Copy link
Contributor Author

martinvuyk commented Mar 10, 2025

I assume at time of writing, magic run benchmarks would use the packaged stdlib instead of the in-tree one?

@Ahajha exactly

A while back (December-ish) the nightly config sections were merged into the normal config sections (so no more distinction between mojo-max-nightly and mojo-max), and these environment variables are there to override the values. So essentially these env vars were just going into the void since that change. Though you also mentioned that the errors have existed longer than that.

Maybe it was around that date that I noticed it not working anymore. I've been using it mostly for PR #3528 and some few others. But since I don't use it that often I can't remember when exactly it broke. But it did work for a while executing it with magic when I added the code in PR #3697 and when PR #3825 was merged

@Ahajha
Copy link
Collaborator

Ahajha commented Mar 10, 2025

I merged a change last week that moved this variable to the non-NIGHTLY variant, could you try rebasing on main and see if it works again?

@martinvuyk
Copy link
Contributor Author

@Ahajha still getting the same error in PR #3528 because I changed the return type of the function in-tree

/mojo/stdlib/benchmarks/collections/bench_string.mojo:117:30: error: cannot implicitly convert 'List[String]' value to 'List[StringSlice[items]]'
            res = items.split(sequence.value())
                  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

@Ahajha
Copy link
Collaborator

Ahajha commented Mar 10, 2025

@martinvuyk Thanks for checking, I'll take a look then.

@Ahajha
Copy link
Collaborator

Ahajha commented Mar 12, 2025

Haven't forgotten about this - I am going to try to get to it this week.

@martinvuyk
Copy link
Contributor Author

#4149

@martinvuyk martinvuyk closed this Mar 19, 2025
@martinvuyk martinvuyk deleted the fix-public-benchmarking branch March 19, 2025 01:48
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.

4 participants