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

Re render #43

Merged
merged 5 commits into from
Jan 30, 2019
Merged

Re render #43

merged 5 commits into from
Jan 30, 2019

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Jan 28, 2019

I'm getting an

ERROR conda.core.link:_execute(543): An error occurred while installing package 'conda-forge::ipykernel-5.1.0-py37h39e3cac_1001'.
PermissionError(13, 'Permission denied')
Attempting to roll back.
Rolling back transaction: ...working... done
[Errno 13] Permission denied: 'C:\\Miniconda36-x64\\envs\\IOOS\\Scripts\\jupyter-kernel.exe'

error. Let's see if this helps.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ocefpaf
Copy link
Member Author

ocefpaf commented Jan 28, 2019

Not sure what to do about the test failures :-/

@bollwyvl
Copy link
Contributor

bollwyvl commented Jan 28, 2019 via email

@bollwyvl
Copy link
Contributor

I think a couple things are happening.

First, at least locally I am getting hit with some PEP 518 stuff like:
conda-forge/entrypoints-feedstock#12 (comment)

(building the wheel and installing that gets me to the tests).

Next, we're definitely seeing some weird leakage into the path environment, which is pretty much guaranteed to break things... perhaps a pytest change?

    def test_sys_path():
        """test that sys.path doesn't get messed up by default"""
        with kernel() as kc:
            msg_id, content = execute(kc=kc, code="import sys; print (repr(sys.path[0]))")
            stdout, stderr = assemble_output(kc.iopub_channel)
>           assert stdout == "''\n"
E           assert "'/home/conda...thon36.zip'\n" == "''\n"
E             - '$PREFIX/lib/python36.zip'
E             + ''

Finally, the completion thing:

 def test_complete():
        with kernel() as kc:
            execute(u'a = 1', kc=kc)
            wait_for_idle(kc)
            cell = 'import IPython\nb = a.'
            kc.complete(cell)
            reply = kc.get_shell_msg(block=True, timeout=TIMEOUT)
            c = reply['content']
            assert c['status'] == 'ok'
>           assert c['cursor_start'] == cell.find('a.')
E           AssertionError: assert 21 == 19
E            +  where 19 = <built-in method find of str object at 0x7fc627643c00>('a.')
E            +    where <built-in method find of str object at 0x7fc627643c00> = 'import IPython\nb = a.'.find

Seems like an actual error, but unlikely due to packaging, etc.

Will keep investigating...

@bollwyvl
Copy link
Contributor

Running with an older pytest did not help...

@bollwyvl
Copy link
Contributor

pushed with ipython<7.2 in the test section, which WFM locally... this definitely needs to be investigated upstream (both in ipython-feedstock, and likely in the source repos themselves).

I guess on the IPython feedstock we should add a downstream test!

We shall see!

@minrk
Copy link
Member

minrk commented Jan 29, 2019

I think the answer is to not run ipykernel's test suite on conda-forge. They do not test the relevant content of this repo, so aren't providing useful feedback. None of the failures here should result in ipykernel not being packaged on conda-forge.

The only tests that make sense to run on conda-forge for me are:

  1. did it build right,
  2. did it install right

For pure-Python packages, exercise of entrypoints and imports (and package data/data_files) seem to be the only tests that make sense to me.

@bollwyvl
Copy link
Contributor

I think the answer is to not run ipykernel's test suite on conda-forge.

I keep hearing this from various quarters on various packages, and I personally wouldn't merge such a PR, but then I'm not the only maintainer 😉 Indeed, I was very possibly the committer of adding them.

While the official guidance has softened some, I'll keep fighting for running as many tests as I can, to the extent that I'll even go out and get them from source so they can be run.

CF automation is our only line of defense against unbounded maintainer effort: when I see a green check on a feedstock that I know runs the tests, I am pretty sure I can press merge and get on with... whatever it is I'd rather be doing. Additionally, every time we find an issue in the build because of tests is one or more end user issues not created here or on the upstream repository for conda-forge-delivered packages when something breaks.

dev_url: https://github.com/ipython/ipykernel
doc_url: https://ipython.readthedocs.io
doc_source_url: https://github.com/ipython/ipykernel/blob/master/docs/index.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh! 🍬

@bollwyvl
Copy link
Contributor

Ok, appveyor (and azure!) appear to be happy with the combination of:

  • testing with ipython<7.2
  • pre-building the wheel
  • not quoting the script line with --prefix (but I've gone ahead and unquoted all of them)

Will do some more looking on the upstreams, but this should be good to go once the rest of the robots concur!

@hmaarrfk
Copy link
Contributor

Has anybody found out how to install ipykernel in the mean time? Neither conda-forge nor defaults seems to install 😕

@hmaarrfk
Copy link
Contributor

This doesn't seem to be ipykernel's issue:

Executing transaction: failed
ERROR conda.core.link:_execute(507): An error occurred while installing package 'conda-forge::vs2015_runtime-14.0.25420-0'.
PermissionError(13, 'Permission denied')

@bollwyvl
Copy link
Contributor

Travis 502ed, but it started almost immediately after re-kicking... 🤞

@hmaarrfk
Copy link
Contributor

So I'm not sure this was ever a ipykernel bug:
conda/conda#8155

Apparently 4.6.2 will fix the issue.

@bollwyvl
Copy link
Contributor

Thanks for the info, @hmaarrfk! And shucks, we just had 💚. I'm fine to wait a moment on merging this one, if we don't think it's going to actually help anybody... will give some time to figure out the upstream stuff a bit...

@hmaarrfk
Copy link
Contributor

I mean, i spent a good hour pinning different packages to old versions, eventually Visual STudio bugged out. I knew it couldn't be ipykernels ....

@ocefpaf ocefpaf merged commit 0737757 into conda-forge:master Jan 30, 2019
@ocefpaf ocefpaf deleted the re-render branch January 30, 2019 09:59
@bollwyvl
Copy link
Contributor

bollwyvl commented Jan 30, 2019 via email

@ocefpaf
Copy link
Member Author

ocefpaf commented Jan 30, 2019

Ah, guess we're doing this thing! Thanks for the review!

It does not solve the issue but there are more changes here we want to keep, like dropping the tests.

recipe/meta.yaml Outdated
@@ -12,8 +12,9 @@ build:
number: 1002
skip: True # [py<34]
script:
- "{{ PYTHON }} -m pip install . --no-deps -vv"
- python -m ipykernel install --prefix {{ PREFIX }}
- "{{ PYTHON }} setup.py bdist_wheel"
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I forgot about this and we should probably go back to the old way in the next release.

bollwyvl added a commit to bollwyvl/ipykernel-feedstock that referenced this pull request Dec 27, 2022
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.

5 participants