MAINT Configure Windows CI using AppVeyor #258

jacksonllee · 2018-06-11T17:19:58Z

IT support has turned on AppVeyor for CI in Windows. This PR configures that, based on what we already have for Circle CI.

Resolves #10.

…ility

by checking just whether or not poller is called. Asserting a specific call count for a given sleep time is not practical, because of different execution/testing environments (Circle CI versus AppVeyor, for example).

Because polling_interval is 0.01, setting sleep time of 0.02s should (hopefully) be long enough to register at least 1 poller call.

feather-format depends on pyarrow, which does not have Windows packages for python version < 3.5. See https://arrow.apache.org/docs/python/install.html

keithing · 2018-06-11T21:47:57Z

civis/tests/test_io.py

                                               polling_interval=POLL_INTERVAL)
                result = result.result()
                cls.export_url = result['output'][0]['path']
                assert result.state == 'succeeded'

            cls.export_job_id = result.sql_id

+    @pytest.mark.skipif(sys.platform.startswith('win'))  # TODO


Did you fix this test? Does it still need to be skipped in Windows?

The test isn't fixed yet, and I plan to. I threw in this skipif thing to suppress for something else. We're very close to getting the whole thing working. I'll tag you when the PR is ready fo review.

Sounds good. Great work on this so far!

The archive name is a member file inside a zip file. It'd seem more reasonable to have it different than the zip file name.

jacksonllee · 2018-06-12T19:33:56Z

civis/tests/test_io.py

+            fname = os.path.join(temp_dir, str(uuid.uuid4()))
+            with zipfile.ZipFile(fname, 'w', zipfile.ZIP_DEFLATED) as zip_file:
+                archive_name = str(uuid.uuid4())
+                zip_file.writestr(archive_name, 'a,b,c\n1,2,3')


archive_name should be its own distinct filename rather than being the same as the zipf file's filename. Also, using the zip file's filename failed this test because apparently the ZipFile object normalizes backslashes into forward slashes in filenames internally, which caused test failures like this.

So this means that you can't use archive_name = os.path.join(temp_dir, str(uuid.uiid4()))? If you run the tests locally, won't this litter your folder?

I've just run the test suite locally to verify that no random temp files are created. zip_file.writestr(archive_name, ...) writes a new member file named archive_name to zip_file which is the already instantiated temp zip file; it doesn't mean creating a file called archive_name at the current directory.

jacksonllee · 2018-06-12T19:39:16Z

civis/tests/test_polling.py

-        time.sleep(0.015)
-        assert poller.call_count == 1
+        time.sleep(0.02)
+        assert poller.call_count > 0


Because now we have different execution environments for builds (Circle CI versus AppVeyor), this test has become a little unstable (see this test failure due to such indeterminancy). Given polling_internal=0.01 (set just a few lines above, via PollableResult), sleeping 0.02s gives a little more time for at least one poller call count.

It was a little unstable in the Linux tests, to be honest. Hopefully this change makes life better for everyone. Thank you!

jacksonllee · 2018-06-12T19:44:14Z

dev-requirements.txt

@@ -7,7 +7,7 @@ vcrpy>=1.11.0,<=1.11.99
 vcrpy-unittest==0.1.6
 sphinx_rtd_theme>=0.2.4,<1.0.0
 numpydoc>=0.7.0,<1.0.0
-feather-format
+feather-format; sys_platform != 'win32' or python_version >= '3.5'


feather-format depends on pyarrow, which doesn't have a Windows installer for Python < 3.5; see here. Not having feather-format installed skips one test only, which I think is fine (and we are running this test for Python >= 3.5 on Windows anyway).

jacksonllee · 2018-06-12T19:46:16Z

@keithing I've defeated AppVeyor! Good news is that only changes in tests are needed for Windows compatibility. It's mostly about not using tempfile.NamedTemporaryFile. I've left comments for other changes. This PR is ready for review.

keithing

LGTM! @stephen-hoover, you on board for adding Windows tests?

stephen-hoover

This is great! I left a couple of comments in places where I think the code could be made a bit cleaner by fully embracing the temporary directory, but I'm happy merging this as is.

stephen-hoover · 2018-06-12T21:58:11Z

civis/ml/tests/test_model.py

-    with tempfile.NamedTemporaryFile() as tempfname:
-        fname = tempfname.name
+    with TemporaryDirectory() as temp_dir:
+        fname = os.path.join(temp_dir, str(uuid.uuid4()))


You don't need a uuid for anything created in a temporary directory -- the file will vanish as soon as you exit the context, and I don't see a danger of a name collision inside this test.

stephen-hoover · 2018-06-12T21:59:59Z

civis/tests/test_io.py

+            with open(fname, 'w+b') as tmp:
+                tmp.write(b'a,b,c\n1,2,3')
+                tmp.flush()
+                tmp.seek(0)


I would exit the write context and re-open the file in a new with open(...) context here. The seek(0) was so that we could use a temporary file which disappeared when closed.

stephen-hoover · 2018-06-12T22:00:49Z

civis/tests/test_io.py

+            fname = os.path.join(temp_dir, str(uuid.uuid4()))
+            with open(fname, 'w+b') as tmp:
+                tmp.write(b'a,b,c\n1,2,3')
+                tmp.flush()


This should be unnecessary now that we're using a write context manager, but it also won't hurt anything.

stephen-hoover · 2018-06-12T22:02:05Z

civis/tests/test_polling.py

-        time.sleep(0.015)
-        assert poller.call_count == 1
+        time.sleep(0.02)
+        assert poller.call_count > 0


It was a little unstable in the Linux tests, to be honest. Hopefully this change makes life better for everyone. Thank you!

jacksonllee · 2018-06-13T12:25:25Z

Hmm, I've made changes according to Stephen's suggestions, but now the builds fail for Python 3.5 and 3.6 (for both Linux and Windows builds). For these two Python versions, it looks like we're using whatever the latest version of pandas is available from PyPI. pandas v0.23.1 was released just 9 hours ago -- either it is broken, or at least it breaks our builds.

Indeed, all builds passed with pandas v0.23.0; see the successful Linux builds triggered by 9fbb4be committed yesterday.

Back to the most recent build failures for Python 3.5 and 3.6, it looks like when _stash_dataframe_as_csv from civis/ml/_model.py calls df.to_csv, the under-the-hood pandas's CSVFormatter object calls the save method, which triggers AttributeError: '_io.TextIOWrapper' object has no attribute 'getvalue'.

I'll come back to this PR later in the day.

stephen-hoover · 2018-06-13T14:13:37Z

This test failure was caused by pandas-dev/pandas#21300 . I'm not sure if it represents a bug on our end or just a change in the parameter options available for to_csv. I think we should fix _stash_dataframe_as_csv in a different PR so that it works with the latest version of pandas.

jacksonllee · 2018-06-13T15:40:49Z

#259 fixes _stash_dataframe_as_csv. When that PR is merged, I'll rebase here.

jacksonllee · 2018-06-13T20:58:51Z

I'm sorting out the repo / AppVeyor settings with IT support. Sit tight, everyone.

jacksonllee · 2018-06-14T15:33:23Z

We're all set. All builds for both Linux and Windows have passed. This PR is ready for review again.

keithing

This is great. Thanks for sticking with this. LGTM.

stephen-hoover

LGTM! Thank you for doing this!

jacksonlee-civis added 2 commits May 29, 2018 11:51

MAINT Configure Windows CI using AppVeyor

e5b4ac1

MAINT Update the changelog

6fd020a

jacksonllee added Testing Maintenance labels Jun 11, 2018

jacksonlee-civis added 10 commits June 11, 2018 12:54

TST NamedTemporaryFile -> TemporaryDirectory etc for Windows compatib…

e35f0fd

…ility

TST Use TemporaryDirectory from compat module

444da8b

TST Avoid accessing 'name' attr of temp file objects

d28d6aa

STY Remove unused imports and vars to appease flake8

353ef3d

TST Close the temp files explicitly

c02f8c0

REF Switch to context managers

35a92b0

TST More time for polling call count test

a211a3f

TST Relax the polling call count test

c142274

by checking just whether or not poller is called. Asserting a specific call count for a given sleep time is not practical, because of different execution/testing environments (Circle CI versus AppVeyor, for example).

TST Adjust sleep time in poller call count test

c93b8f2

Because polling_interval is 0.01, setting sleep time of 0.02s should (hopefully) be long enough to register at least 1 poller call.

DEP No feather-format on Windows when python version < 3.5

4be1616

feather-format depends on pyarrow, which does not have Windows packages for python version < 3.5. See https://arrow.apache.org/docs/python/install.html

keithing reviewed Jun 11, 2018

View reviewed changes

TST pytest skipif needs a reason like a human

b7d8fb7

jacksonllee added this to the Next Version milestone Jun 11, 2018

jacksonlee-civis added 3 commits June 12, 2018 09:18

TST Use a distinct archive name in zipfile testing

298e176

The archive name is a member file inside a zip file. It'd seem more reasonable to have it different than the zip file name.

TST Remove skipif for Windows

8474aac

STY Remove unused import #flake8

9fbb4be

jacksonllee commented Jun 12, 2018

View reviewed changes

jacksonllee requested a review from keithing June 12, 2018 19:46

jacksonllee assigned keithing Jun 12, 2018

jacksonllee changed the title ~~WIP MAINT Configure Windows CI using AppVeyor~~ MAINT Configure Windows CI using AppVeyor Jun 12, 2018

keithing previously approved these changes Jun 12, 2018

View reviewed changes

stephen-hoover previously approved these changes Jun 12, 2018

View reviewed changes

jacksonlee-civis added 2 commits June 13, 2018 06:23

TST uuid is not needed

2f2413d

TST flushing not needed when existing file context manager

e294c36

jacksonllee dismissed stephen-hoover’s stale review via e294c36 June 13, 2018 11:39

jacksonllee dismissed keithing’s stale review via e294c36 June 13, 2018 11:39

jacksonllee mentioned this pull request Jun 13, 2018

BUG A file-like object as arg to df.to_csv must have the method getvalue #259

Merged

jacksonllee and others added 2 commits June 13, 2018 15:16

Merge branch 'master' into set-up-windows-continuous-integration

80fb27d

MAINT Update the changelog

6bed89f

jacksonllee mentioned this pull request Jun 13, 2018

MAINT Configure Windows CI using AppVeyor #260

Closed

jacksonllee assigned jacksonllee and unassigned keithing Jun 13, 2018

MAINT Fix PR number in changelog

46c5fca

jacksonllee assigned stephen-hoover and keithing and unassigned jacksonllee Jun 14, 2018

keithing approved these changes Jun 14, 2018

View reviewed changes

stephen-hoover approved these changes Jun 14, 2018

View reviewed changes

stephen-hoover assigned jacksonllee and unassigned keithing and stephen-hoover Jun 14, 2018

jacksonllee merged commit c220a52 into civisanalytics:master Jun 14, 2018

jacksonllee deleted the set-up-windows-continuous-integration branch June 14, 2018 17:13

MAINT Configure Windows CI using AppVeyor #258

MAINT Configure Windows CI using AppVeyor #258

Uh oh!

Conversation

jacksonllee commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithing Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithing Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacksonllee Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacksonllee Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacksonllee commented Jun 12, 2018

Uh oh!

keithing left a comment

Choose a reason for hiding this comment

Uh oh!

stephen-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacksonllee commented Jun 13, 2018

Uh oh!

stephen-hoover commented Jun 13, 2018

Uh oh!

jacksonllee commented Jun 13, 2018

Uh oh!

jacksonllee commented Jun 13, 2018

Uh oh!

jacksonllee commented Jun 14, 2018

Uh oh!

keithing left a comment

Choose a reason for hiding this comment

Uh oh!

stephen-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithing Jun 11, 2018 •

edited

Loading

keithing Jun 12, 2018 •

edited

Loading

jacksonllee Jun 12, 2018 •

edited

Loading

jacksonllee Jun 12, 2018 •

edited

Loading