hobuinc / mgrs

Python MGRS library
https://pypi.python.org/pypi/mgrs
MIT License
111 stars 36 forks source link

Update pep425tags import to reflect move to pip #31

Closed eHildy closed 4 years ago

eHildy commented 4 years ago

We suddenly had a problem creating venvs for a project using a known-good set of instructions. After some investigating, the problem turned out to be that when our code ran mgrs, it was trying to import the module "libmgrs.pyd," when the actual file is called "libmgrs.cp37-win_amd64.pyd."

What was causing this is that pep425tags has recently moved from wheel, and into pip; I honestly haven't had time to figure out exactly when, but we're manually rolling wheel back to 0.33.x in our venvs for now. Wheel is now at 0.35.x, so it happened sometime in between.

This was an easy fix, so I just did this PR for you, rather than raise an issue.

hobu commented 4 years ago

Thanks for the pull request. The Windows builder is now failing with this change, however. IIRC, I had made accommodations in the windows build to load the pyd using the pep425tags. A very-current 1.3.8 MGRS doesn't work on windows for you? The builders and tests working on windows there, but maybe we don't have good enough coverage.

What was causing this is that pep425tags has recently moved from wheel, and into pip

Can you describe this scenario more completely?

eHildy commented 4 years ago

Yes, that's correct, mgrs 1.3.8 does not work for our Windows machines: 3 machines tested so far.

As for describing it more completely, I'm not sure what else to say. We traced the problem down, and the fact pep425tags no longer exists in wheel is what is causing it. That function will just just construct 'libmgrs.pyd.' If your Windows build is failing, I suspect it's not using wheel 0.35, and is using an older version where pip does not yet own pep425tags.

I could perhaps ad a wheel version check and conditionally do the import.

hobu commented 4 years ago

the fact pep425tags no longer exists in wheel

in MGRS's wheel, or all wheels?

I could perhaps ad a wheel version check and conditionally do the import.

Yes, maybe we need to do this. I'll admit to not understanding the shared library rules in wheels anyway.

If you get this fixed such that all the builders are green and you're happy with it too, I'll quickly snap a new release for you.

eHildy commented 4 years ago

As of today, it's just mgrs. We're lucky enough that this is the only package using something that was moved to pip...for now. I'll see what I can figure out.

eHildy commented 4 years ago

Ok, I added some checks for the location of pep425tags. I has to add an import, so I organized those alphabetically, as is standard. I also added a docstring to the function in question. Fingers crossed this passes...because I gotta get back to work lol.

hobu commented 4 years ago

It looks like it still tried to load the untagged pyd on the windows builder https://github.com/hobu/mgrs/pull/31/checks?check_run_id=1182467443#step:7:48

mgrs.core.MGRSError: Unable to load libmgrs.pyd

eHildy commented 4 years ago

Yeah, that's the same thing that made it die on our local machines. The fix I pushed works for venvs constructed from PyPI, but it's dying on a local env I created from Conda, which is puzzling. It's actually not finding pep425tags in wheel or pip.

eHildy commented 4 years ago

I see what's happening in Conda, just not sure what to do about it. pep425tags does not exists anywhere in a Conda environment; I've searched in wheel, pip, then in the whole environment.

So when _load_library() calls get_windows_platform_name(), the result of that is always going to be "libmgrs.pyd." When that is used in the recursice call to _load_library(), it will always fail.

eHildy commented 4 years ago

I have something else to try I'm about to push up. I also found a bug in the way you're checking for a Conda.

eHildy commented 4 years ago

Looks like your linter in a couple of your Ubuntu images are fussy. I thought I saw a commit message mentioning some flake updates; did those make it into said images?

The miniconda build is still weird. I don't have miniconda installed to debug that.

hobu commented 4 years ago

Indeed the linter is fussy. I just use stock flake8 though.

mgrs/core.py:29:42: E999 SyntaxError: invalid syntax

☝️ seems like a real error on the ubuntu builder though.

eHildy commented 4 years ago

Right, but that is valid syntax, and it's happening in the lint step. What's add is that same string terp syntax is working elsewhere.

hobu commented 4 years ago

Right, but that is valid syntax, and it's happening in the lint step. What's add is that same string terp syntax is working elsewhere.

Older python or something maybe?

eHildy commented 4 years ago

It says they were introduced in Python 3.6, which I think is the oldest thing you have, so it should be good. It's weird that one Ubuntu passes though.

eHildy commented 4 years ago

Excuse me, by "they" I mean f-strings

hobu commented 4 years ago

Excuse me, by "they" I mean f-strings

Did their syntax change in anyway between Python 3.6 and 3.7? Just WAG'ing.

eHildy commented 4 years ago

No, but I did find this, which could explain why some Ubuntu images are working and others aren't:

https://github.com/AtomLinter/linter-flake8/issues/580#issuecomment-407135338

eHildy commented 4 years ago

Pushing an update to see what file that failed conda build is trying to load. The c-style string terp wasn't working.

eHildy commented 4 years ago

Pushing an update to see what file that failed conda build is trying to load. The c-style string terp wasn't working.

Ok, it looks like MiniConda perhaps does not construct its environments the same way as full Conda. So looking for the glob in Lib/site-packages likely just returns nothing.

I'm going to push up some print statements to test a hypothesis...

hobu commented 4 years ago

I'm going to push up some print statements to test a hypothesis...

Lint away! I appreciate the effort.

btw, you can modify the builders in .github/workflows if you think it is necessary.

eHildy commented 4 years ago

...and the Windows one I needed cancelled lol.

For the Ubuntu thing, I'm pretty certain you just need to update flake. Just know that doing pip install flake will not automatically update the package. You'd have to do pip install --upgrade flake

Bet that takes care of it, based on that bug report.

hobu commented 4 years ago

update it here https://github.com/hobu/mgrs/blob/master/.github/workflows/build.yml#L70

hobu commented 4 years ago

btw, it auto-cancels everything at the first failure, so you have to knock out the failure to get everything to build.

eHildy commented 4 years ago

btw, it auto-cancels everything at the first failure, so you have to knock out the failure to get everything to build.

Looks like the windows-latest died trying to update Conda this time

eHildy commented 4 years ago

The output from when I updated that runner revealed that at least some of your Ubuntu images are using Python 3.5, which doesn't support f-strings and is deprecated. It also says pip will stop supporting in in Jan 2021.

eHildy commented 4 years ago

The output from when I updated that runner revealed that at least some of your Ubuntu images are using Python 3.5, which doesn't support f-strings and is deprecated. It also says pip will stop supporting in in Jan 2021.

I tried adding this to the build, but I've never done this in GitHub, so fingers crossed lol.

eHildy commented 4 years ago

If you look at the failed ubuntu-16.04 build, in the build stage, it's still trying to use 3.5

eHildy commented 4 years ago

If you look at the failed ubuntu-16.04 build, in the build stage, it's still trying to use 3.5

Ok, so I apparently upset the GitHub gods, and my pushes aren't triggering builds. I'll have to check back later.

hobu commented 4 years ago

If you look at the failed ubuntu-16.04 build, in the build stage, it's still trying to use 3.5

Ok, so I apparently upset the GitHub gods, and my pushes aren't triggering builds. I'll have to check back later.

yaml syntax is invalid. maybe you mixed tabs in there? I'll catch up to this and make a few tweaks...

eHildy commented 4 years ago

If you look at the failed ubuntu-16.04 build, in the build stage, it's still trying to use 3.5

Ok, so I apparently upset the GitHub gods, and my pushes aren't triggering builds. I'll have to check back later.

yaml syntax is invalid. maybe you mixed tabs in there? I'll catch up to this and make a few tweaks...

Looks ok on my end. I just copy/pasted the with clause from elsewhere.

hobu commented 4 years ago

hmph, I made a push to your PR and it didn't run the CI

eHildy commented 4 years ago

hmph, I made a push to your PR and it didn't run the CI

let me try something...

eHildy commented 4 years ago

hmph, I made a push to your PR and it didn't run the CI

let me try something...

That didn't work. Something else is stopping it. Is there like a daily limit on how much you can run before you have to pay or something?

hobu commented 4 years ago

I don't think there's a limit like that.

Maybe try opening a new PR?

eHildy commented 4 years ago

I don't think there's a limit like that.

Maybe try opening a new PR?

That might be what we need, but I'm going to try rolling back all yaml changes real quick.

eHildy commented 4 years ago

Yeah, I jacked up the yaml...it's running now

eHildy commented 4 years ago

Would you mind updating the yaml to use 3.6+? I know that will fix the Ubuntu build, and I can return to the remaining Windows build issue.

hobu commented 4 years ago

just the conda windows error now

eHildy commented 4 years ago

just the conda windows error now

Sweet, let me see if I can figure that one out.

eHildy commented 4 years ago

just the conda windows error now

Sweet, let me see if I can figure that one out.

Ok, so that's a no-go. I completely removed conda from my machine and installed miniconda. I found that the file structure is exactly the same, and that my code for detecting conda runs perfectly locally (just like it does on your other Windows builds). This was, however, installing mgrs via pip. I found that I can't actually install mgrs via conda install -c conda-forge mgrs. The thing that prevents me from doing so may not be exactly the same as your build machine, but perhaps it's similar.

The eaxct error I get trying to install mgrs via conda is: **The following specifications were found to be incompatible with your CUDA driver:

Your installed CUDA driver is: 9.1**

Again, the cuda driver part is unlikely in your build machine, but you may want to confirm that it's not failing because it can't install mgrs from conda forge.

eHildy commented 4 years ago

Shoot me an email when you get that part worked out, as I'm at the limit of the time I can spend on this for now.

...good luck!

eHildy commented 4 years ago

You are absolutely the man. I just created a new venv and mgrs seems to be good to go! Thanks so much!