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

Test on Python 3.9 in CI #10550

Merged
merged 11 commits into from Dec 2, 2020
Merged

Test on Python 3.9 in CI #10550

merged 11 commits into from Dec 2, 2020

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Oct 6, 2020

No description provided.

@mattpap mattpap added this to the 2.3 milestone Oct 6, 2020
@mattpap
Copy link
Contributor Author

mattpap commented Oct 6, 2020

The unit test failure we've been seeing is due to pytest's upgrade from 6.0 to 6.1. There were quite a few changes related to warnings in 6.1, but at this point I'm not sure if what we are observing is a bug or a feature.

@bryevdv
Copy link
Member

bryevdv commented Oct 6, 2020

Lots of pytest-related churn lately, I had to pin pytest-asyncio recently too. Suggest pinning for now and making an issue to follow up

@bryevdv
Copy link
Member

bryevdv commented Oct 13, 2020

rebased on branch-2.3 for mypy pin

@mattpap mattpap force-pushed the mattpap/python3.9 branch 2 times, most recently from a9d9e56 to 812f66a Compare November 2, 2020 12:10
@mattpap
Copy link
Contributor Author

mattpap commented Nov 17, 2020

This is what works:

$ git df
diff --git a/ci/environment-test-3.9.yml b/ci/environment-test-3.9.yml
index be5712302..9efcdfb94 100644
--- a/ci/environment-test-3.9.yml
+++ b/ci/environment-test-3.9.yml
@@ -28,19 +28,19 @@ dependencies:
   - flaky
   - geckodriver
   - ipython
-  - isort <5.0
+  - isort
   - mock
-  - mypy <=0.782
+  - mypy
   - nbconvert >=5.4
   - networkx
   - nodejs 14.*
-  - numba
+  #- numba
   - pandas
   - psutil
   - pydot
   - pygments
-  - pytest 6.0*
-  - pytest-asyncio=0.12.0
+  - pytest
+  - pytest-asyncio
   - pytest-cov >=1.8.1
   - pytest-html
   - pytest-xdist

Essentially we have to fix all the pins and wait for numba.

@mattpap
Copy link
Contributor Author

mattpap commented Nov 17, 2020

Python 3.8 and 3.9 disagree here (same dependencies):

___________________________________________________________ test__exclude_headers ___________________________________________________________

ManagedServerLoop = <function ManagedServerLoop.<locals>.msl at 0x7fd300062b80>

    async def test__exclude_headers(ManagedServerLoop) -> None:
        application = Application()
        with ManagedServerLoop(application, exclude_headers=['Connection', 'Host']) as server:
            sessions = server.get_sessions('/')
            assert 0 == len(sessions)
    
            response = await http_get(server.io_loop, url(server))
            html = response.body
            token = extract_token_from_json(html)
            payload = get_token_payload(token)
            assert 'headers' in payload
>           assert payload['headers'] == {'Accept-Encoding': 'gzip'}
E           AssertionError: assert {'Accept-Encoding': 'gzip', 'User-Agent': 'Tornado/6.1'} == {'Accept-Encoding': 'gzip'}
E             Common items:
E             {'Accept-Encoding': 'gzip'}
E             Left contains 1 more item:
E             {'User-Agent': 'Tornado/6.1'}
E             Full diff:
E             - {'Accept-Encoding': 'gzip'}
E             + {'Accept-Encoding': 'gzip', 'User-Agent': 'Tornado/6.1'}

tests/unit/bokeh/server/test_server__server.py:346: AssertionError

@bryevdv
Copy link
Member

bryevdv commented Nov 17, 2020

Maybe tornado changed for 3.9? If so could just check less strictly for just the presence of the encoding header

@bryevdv
Copy link
Member

bryevdv commented Nov 29, 2020

With this isort config change:

-line_length=88
+line_length=165
+multi_line_output=3

Then most of the new differences appear to be from isort wanting to add section headers inside functions. I have not foudn a way to disable this and only have headers at the top of a module, so I opened an issue to ask about it here PyCQA/isort#1604

@bryevdv
Copy link
Member

bryevdv commented Nov 29, 2020

@mattpap I guess the one warnings.filter test error is down to some pytest interference? When I try the test code manually it works (of course). I suppose we could skip the test for now and make an issue to follow up

@bryevdv
Copy link
Member

bryevdv commented Nov 30, 2020

@mattpap think the ability to turn off in-function headings will be a future enhancement for isort. Can we not just keep using isort 4.x for now? (pip installing if we have to?)

That and skipping the one test would get this passing I think

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2020

@mattpap not sure why mypy suddenly failing. This diff fixes, maybe there is a better fix tho

diff --git a/bokeh/models/tools.py b/bokeh/models/tools.py
index 176cadc8a..2d3861add 100644
--- a/bokeh/models/tools.py
+++ b/bokeh/models/tools.py
@@ -275,25 +275,25 @@ class Toolbar(ToolbarBase):

     '''

-    active_drag: tp.Union[Literal["auto"], Drag] = Either(Auto, Instance(Drag), help="""
+    active_drag: tp.Union[None, Literal["auto"], Tool] = Either(Auto, Instance(Drag), help="""
     Specify a drag tool to be active when the plot is displayed.
     """)

-    active_inspect: tp.Union[Literal["auto"], InspectTool, tp.Sequence[InspectTool]] = \
+    active_inspect: tp.Union[None, Literal["auto"], Tool, tp.Sequence[InspectTool]] = \
         Either(Auto, Instance(InspectTool), Seq(Instance(InspectTool)), help="""
     Specify an inspection tool or sequence of inspection tools to be active when
     the plot is displayed.
     """)

-    active_scroll: tp.Union[Literal["auto"], Scroll] = Either(Auto, Instance(Scroll), help="""
+    active_scroll: tp.Union[None, Literal["auto"], Tool] = Either(Auto, Instance(Scroll), help="""
     Specify a scroll/pinch tool to be active when the plot is displayed.
     """)

-    active_tap: tp.Union[Literal["auto"], Tap] = Either(Auto, Instance(Tap), help="""
+    active_tap: tp.Union[None, Literal["auto"], Tool] = Either(Auto, Instance(Tap), help="""
     Specify a tap/click tool to be active when the plot is displayed.
     """)

-    active_multi: tp.Union[Literal["auto"], GestureTool] = Instance(GestureTool, help="""
+    active_multi: tp.Union[None, Literal["auto"], Tool] = Instance(GestureTool, help="""
     Specify an active multi-gesture tool, for instance an edit tool or a range
     tool.

diff --git a/bokeh/plotting/_tools.py b/bokeh/plotting/_tools.py
index a766d8eb9..2178a44da 100644
--- a/bokeh/plotting/_tools.py
+++ b/bokeh/plotting/_tools.py
@@ -57,10 +57,10 @@ __all__ = (

 # TODO: str should be literal union of e.g. pan | xpan | ypan
 Auto = Literal["auto"]
-ActiveDrag = Union[Drag, Auto, str, None]
-ActiveInspect = Union[List[InspectTool], InspectTool, Auto, str, None]
-ActiveScroll = Union[Scroll, Auto, str, None]
-ActiveTap = Union[Tap, Auto, str, None]
+ActiveDrag = Union[Drag, Auto, None]
+ActiveInspect = Union[List[InspectTool], InspectTool, Auto, None]
+ActiveScroll = Union[Scroll, Auto, None]
+ActiveTap = Union[Tap, Auto, None]

 def process_active_tools(toolbar: Toolbar, tool_map: Dict[str, Tool],
         active_drag: ActiveDrag, active_inspect: ActiveInspect, active_scroll: ActiveScroll, active_tap: ActiveTap)

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2020

One spurious failure trying to restart but GH Actions is having a service disruption currently

@mattpap
Copy link
Contributor Author

mattpap commented Dec 1, 2020

One spurious failure trying to restart but GH Actions is having a service disruption currently

I restarted multiple times before. I doesn't work and looks like something new.

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2020

Re-running to get logs. Will try to compare package versions tonight, not sure what else it could be (and presumably pytest-asyncio)

@bryevdv
Copy link
Member

bryevdv commented Dec 2, 2020

Well, that was like pulling teeth... 🎉

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.

None yet

2 participants