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

Remove --fiximports helper #39274

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

tobiasdiez
Copy link
Contributor

It did its job; the remaining imports are okay (or would need manual work anyway). This is part of trying to eliminate most in misc.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

It did its job; the remaining imports are okay (or would need manual work anyway). This is part of trying to eliminate most in `misc`.
Copy link

github-actions bot commented Jan 5, 2025

Documentation preview for this PR (built with commit df2d827; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor

looks like some of the CI failures are for real this time

@orlitzky
Copy link
Contributor

Unfortunately it looks like the simpler __module__ approach fails in some cases where import_statements would work, e.g.

sage: ZZ.__module__
'sage.categories.category'
sage: import_statements(ZZ)
# ** Warning **: several names for that object: Z, ZZ
from sage.rings.integer_ring import Z
sage: RDF.__module__
'sage.categories.category'
sage: import_statements(RDF)
from sage.rings.real_double import RDF

So maybe that is worth keeping? I don't know of an easier way to get the right import. I always forget about import_statements and do foo?? followed by grep and reading the source.

I don't know what runsnake is, but I guess it should be deprecated first, moreso in this case to find out if anyone is actually using it. It looks like a very thin wrapper around %prun -T file.txt <command> and then running runsnake file.txt in a terminal. (And could probably be replaced by some profiling documentation that says as much.) That would also address the fact that runsnake() uses a temporary file for your profiling data, and therefore deletes it as soon as sage exits.

(The replace_dot_all module is technically public, too, but I can't imagine anyone trying to use it in their own code.)

@tobiasdiez
Copy link
Contributor Author

So maybe that is worth keeping? I don't know of an easier way to get the right import. I always forget about import_statements and do foo?? followed by grep and reading the source.

Okay, I've restored the import_statements methods.

I don't know what runsnake is, but I guess it should be deprecated first, moreso in this case to find out if anyone is actually using it.

The global import of runsnake was deprecated in #34259, so I think it should be safe to delete it.

@orlitzky
Copy link
Contributor

orlitzky commented Mar 7, 2025

The global import of runsnake was deprecated in #34259, so I think it should be safe to delete it.

That only removed it from the global namespace, though -- it's still documented: https://github.com/sagemath/sage/blob/develop/src/doc/en/thematic_tutorials/profiling.rst?plain=1#L79

In case anyone is using it, I think we should update the docs to say "install runsnake and call it on your %prun -T output" rather than suggesting the sage function runsnake(cmd). Then we could deprecate the module itself and point people at the new docs.

@tobiasdiez
Copy link
Contributor Author

Okay, I've restored the runsnake method now and properly deprecated it.

@orlitzky
Copy link
Contributor

Weird to see the CI green.

@tobiasdiez
Copy link
Contributor Author

tobiasdiez commented Mar 11, 2025

Thanks for the review!

Weird to see the CI green.

You can thank @user202729 for this, who recently fixed quite a few issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants