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

MAINT: Fixup and remove bundled dependencies #1234

Merged
merged 16 commits into from
Feb 27, 2023

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Jan 3, 2023

This supersedes #1225 and #1211.

Also closes #1139.

cc @mattip

@HaoZeke HaoZeke force-pushed the noExternal branch 2 times, most recently from 898c148 to 3f39e6f Compare January 3, 2023 18:06
@HaoZeke
Copy link
Member Author

HaoZeke commented Jan 4, 2023

Will debug soon.

@HaoZeke HaoZeke marked this pull request as draft January 4, 2023 17:39
@HaoZeke HaoZeke force-pushed the noExternal branch 2 times, most recently from ab99396 to 689edee Compare February 12, 2023 20:28
@mattip
Copy link
Contributor

mattip commented Feb 19, 2023

This could use a refresh

asv/benchmark.py Outdated
return

try:
subprocess.check_output(['python', "-mpip", "install", 'pympler'])
Copy link
Member Author

@HaoZeke HaoZeke Feb 20, 2023

Choose a reason for hiding this comment

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

This works with all three environment backends, conda, mamba (as of #1238) and virtualenv but is syntactically less than ideal. A better approach would be to modify the constraints of the environment class which is the base of all the backends.. Though YAGNI probably.

@mattip
Copy link
Contributor

mattip commented Feb 23, 2023

After the move from setup.cfg to pyproject.toml this now has conflicts.

MAINT: Kill extern in favor of pympler and pyjson5

Revert "MAINT: Kill extern in favor of pyjson5"

This reverts commit 02a2b170910f55072a008cc7d52da55e0b894b32.

MAINT: Fixup

MAINT: Use pyjson5 correctly
@HaoZeke HaoZeke force-pushed the noExternal branch 3 times, most recently from 0f5aa42 to bbd8f8a Compare February 25, 2023 19:32
@HaoZeke HaoZeke force-pushed the noExternal branch 10 times, most recently from b8bb50a to 068546d Compare February 25, 2023 21:30
@HaoZeke HaoZeke force-pushed the noExternal branch 2 times, most recently from 6c443ea to bb87d6d Compare February 26, 2023 00:22
@HaoZeke HaoZeke force-pushed the noExternal branch 2 times, most recently from 9d85a5f to b2c6f9a Compare February 26, 2023 00:35
@HaoZeke HaoZeke marked this pull request as ready for review February 26, 2023 00:58
return

def import_asizeof():
"""Import asizeof, searching system Pythons in PATH."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need all this? If the import fails, can't we just fail the run?

Copy link
Member Author

@HaoZeke HaoZeke Feb 26, 2023

Choose a reason for hiding this comment

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

The problem here is that the import fails where it shouldn't, e.g. on the CI. This seems to be because of the process isolation (each ASV run is in a different subprocess, which doesn't include asv and on CI cannot use pip either). Perhaps, now that I'm thinking about it, it would be cleaner to instead import asv itself, which would automatically bring in pympler and is discussed in #908

Copy link
Member Author

Choose a reason for hiding this comment

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

If I went with that approach then the Import Mechanism would instead load ASV rather than asizeof, but I'm not sure how to simplify this further without breaking the subprocess approach.

@mattip
Copy link
Contributor

mattip commented Feb 27, 2023

I am fine to merge this as-is and then to iterate on solving the import problem, what do you think?

@HaoZeke
Copy link
Member Author

HaoZeke commented Feb 27, 2023

I am fine to merge this as-is and then to iterate on solving the import problem, what do you think?

Sure that sounds great.

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.

DEPS: dependency on pympler
2 participants