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

BLD: line profiler no longer requires ipython, unpin needed for py3.12 #353

Merged
merged 7 commits into from
Jan 23, 2025

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Jan 9, 2025

Apparently line-profiler hasn't strictly required ipython for a while, and at least locally it seems like I can install 4.1.2 while pinning ipython at 8.4.0.

Curiosity getting me here since I don't want to do other more "productive" things

@tangkong
Copy link
Contributor Author

tangkong commented Jan 9, 2025

It seems the ipython - line-profiler pin is defined in the conda recipe, but pip allows you to install line-profiler without it. I tried moving to the pip installation to avoid this entirely
https://github.com/conda-forge/line_profiler-feedstock/blob/d811524f0cadca17537bd5d7a987f19644781751/recipe/meta.yaml#L39

@ZLLentz
Copy link
Member

ZLLentz commented Jan 9, 2025

If this ends up being a blocker it's probably ok to update ipython to a more recent version

@tangkong
Copy link
Contributor Author

tangkong commented Jan 9, 2025

The scientists scare me 😨 Looking at line profiler it's really just ipython magics that we don't use (but probably should).

I was wondering if we'd end up wanting to scream loudly into the abyss to try and get scientists to try the new ipython version. Maybe that's a fool's errand and we'll get yelled at anyway

@tangkong
Copy link
Contributor Author

tangkong commented Jan 9, 2025

Ok I think this is what's happening, but zach the env master should correct me if I'm wrong.

py312 next-full Log failure

error    libmamba Could not solve for environment specs
    The following package could not be installed
    └─ line-profiler =* * does not exist (perhaps a typo or a missing channel).

Running create_base_env.sh test pcds 3.12 (workflow link)

We first create a base environment with the packages listed in conda-packages.txt and security packages.txt (src)

After this, we "get extra dependencies", and install those via conda/mamba (src)

line-profiler does not exist on conda-forge, but line_profiler does.

I think one solution could be changing happi to use the underscore version of line_profiler pcdshub/happi#352

@ZLLentz
Copy link
Member

ZLLentz commented Jan 9, 2025

I think the underscore solution is reasonable and your analysis is correct
Maybe there's other ways too (unclear) but this works for sure

@tangkong
Copy link
Contributor Author

tangkong commented Jan 9, 2025

happi has been tagged, we now wait for conda-forge to tick over

@tangkong
Copy link
Contributor Author

It's running!

I do find it confusing that we gather extras based on the pip specifications, which we then use to pick packages to install in conda. We should probably check conda's list of extra dependencies, but maybe that's easier said than done

@tangkong
Copy link
Contributor Author

It looks like the Binstar client does not return any of the test dependencies from the conda recipe. I think we typically don't keep those up-to-date anyway, so I'll give up on that little investigation

@tangkong
Copy link
Contributor Author

Failures are pyca (expected failures), and typhos. With a bit of poking over lunch, I hacked together a py312 environment and tried to replicate the package versions from the CI. I could not replicate the segfault unfortunately. The last test before the segfault is the cli-profiler test, which is indeed the subject of this PR, but I still can't seem to replicate the failure.

This feels close though, despite the flagrant disregard for line_profiler's conda ipython pins

@ZLLentz
Copy link
Member

ZLLentz commented Jan 14, 2025

Do you want to claim forward progress and merge then?

@tangkong
Copy link
Contributor Author

I'm not entirely sure I'm happy with this, unless we're pushing for an env I'd rather try to sort out the line-profiler issues all together. I think there's a world where this style of unpinning isn't correct, though I don't exactly have an alternative idea about it.

@tangkong
Copy link
Contributor Author

Without falling off the deep end tracing line_profiler's dependencies, I can't exactly pinpoint why line_profiler has such strict ipython requirements. With ipython 8.4.0 and line_profiler 4.2.0, I can still run the ipython %lprun magics and get reasonable output. 🤷

I think I'll declare this good enough for now, with an additional comment describing what went on

@tangkong tangkong requested a review from ZLLentz January 23, 2025 02:11
@tangkong
Copy link
Contributor Author

tangkong commented Jan 23, 2025

Also maybe kicking this pulls new pyca and changes some of the ci?

@tangkong
Copy link
Contributor Author

pcdswidgets will probably need a tag for the extra deps fix to go through. I'll claim forward progress here and ask for a review

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

Forward progress acknowledged!

@tangkong tangkong merged commit 3f83368 into pcdshub:master Jan 23, 2025
3 of 9 checks passed
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.

2 participants