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

CI: Linting with azure instead of travis #22854

Merged
merged 120 commits into from
Dec 3, 2018
Merged

CI: Linting with azure instead of travis #22854

merged 120 commits into from
Dec 3, 2018

Conversation

datapythonista
Copy link
Member

@datapythonista datapythonista commented Sep 27, 2018

closes #22844

Moving all the linting, other code checks and doctests to azure.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

I'm interested to see what the reporting looks like. Could you introduce a few changes that fail each one?

In particular, we want all of the steps to run, even if an early one fails.

For checks longer than a line, I think making a collections of scripts in ci/lint/ makes sense. Then in the azure config we call - script: ./ci/lint/sphinx_directives.sh.

#C410 # Unnecessary (list/tuple) passed to list() - (remove the outer call to list()/rewrite as a list literal).

# pandas/_libs/src is C code, so no need to search there.
- script: flake8 pandas --filename=*.py --exclude pandas/_libs/src --ignore=C406,C408,C409,E402,E731,E741,W503
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this --ignore necessary? I would think that this would be picked up from our setup.cfg. Better to delete it here so that we don't have to update two places.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. That was in the original lint.sh, but will check if the setting in setup.cfg is used.

@datapythonista
Copy link
Member Author

Looks like this requires a bit more research. I'm not activating the conda environment, so I was guessing whether this would fail or not. And it seems it does.

This answers your question. Defining them as steps, when one fails, the job is interrupted. An alternative would be to create a job for each, but I think that will create too many jobs, and it'll make it difficult to find the jobs of the tests. Will do some research and see if it's possible to continue the steps when one is failing.

I think yaml should support having scripts with multiple lines, I'll give that a try. I think that should be neater than having a script for each check with a loop.

@TomAugspurger
Copy link
Contributor

https://docs.microsoft.com/en-us/azure/devops/pipelines/yaml-schema?view=vsts#job

Looks like there's a continueOnError field that determines this. That can be set at the job or script level. We'll want true.

…s that used to be loops). Installing missing packages, splitting linting and checks, and continuing on errors
@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #22854 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22854      +/-   ##
==========================================
- Coverage   42.45%   42.44%   -0.01%     
==========================================
  Files         161      161              
  Lines       51561    51559       -2     
==========================================
- Hits        21888    21886       -2     
  Misses      29673    29673
Flag Coverage Δ
#single 42.44% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/panel.py 42.02% <ø> (ø) ⬆️
pandas/core/tools/timedeltas.py 66.66% <ø> (ø) ⬆️
pandas/core/frame.py 38.7% <ø> (ø) ⬆️
pandas/io/json/json.py 16.66% <ø> (ø) ⬆️
pandas/io/gbq.py 25% <ø> (ø) ⬆️
pandas/core/window.py 28.93% <100%> (ø) ⬆️
pandas/core/arrays/period.py 36.97% <0%> (-0.15%) ⬇️
pandas/core/arrays/datetimes.py 63.41% <0%> (-0.08%) ⬇️
pandas/core/strings.py 33% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45f880b...498cebb. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Sep 27, 2018

Hello @datapythonista! Thanks for updating the PR.

Comment last updated on November 12, 2018 at 16:58 Hours UTC

@datapythonista
Copy link
Member Author

@TomAugspurger seems like the continueOnError is not working at the job level (it should based on the docs), it needs to be at step level. But more important, continuing means that the build will be successful when an step fails, so it's not an option.

I think the log is much clearer, I personally thing it'd be great if we could use azure for the linting. But I think the drawbacks are probably deal breakers, and I don't think they can be fixed:

  • We can't run all the linting/checks, seems like when one in each job fails it has to stop. And I don't think we want to have one job per test, that would make things much more complicated (both in the dashboard and in the code).
  • Having the checking in the yaml, we can't run ./ci/lint.sh locally. We could have a script that parses the yaml and executes all the checks, or we could build the yaml from a script. But I don't think any of them is actually worth.

Thoughts?

CC @chrisrpatterson

@datapythonista datapythonista changed the title WIP: Moving one-line linting and checks from lint.sh to azure steps WIP: Linting with azure instead of travis (tests to see how it looks like) Sep 29, 2018
@datapythonista
Copy link
Member Author

Looks like continueOnError is converting the error in a warning, but adding condition: true is what makes that a step is executed no matter the outcome of the previous.

This looks cool now:
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=367&view=logs

And seems like we can have links to the source code line with the linting error, which may also be helpful in some cases.

I guess we still want to be able to run all the linting and checks locally. I think a script that parses the yaml and execute every command on it should be really easy, and would do the work for now.

Thoughts? @TomAugspurger @jreback

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 29, 2018 via email

@datapythonista
Copy link
Member Author

I unified all the linting of *.py files in #22863 (adcda47), which I think that simplifies things, and adds few value to have them separate, even in azure.

But even with that we'd have more than 10 scripts. And while that would surely guarantee the syncronization between azure and lint.sh in the exact commands, I think we can end up having differences on which scripts are called in each place. And, I think we'd loose the command being executed (e.g. we'd see in the logs ./ci/check_deprecated_directive.sh instead of grep -v ".. deprecated::, which I think makes things more difficult to debug).

So, I think I prefer another option (which look much simpler to me):

  • We could simply not have anything to call all checks (linting .py files is as simple as flake8 . with the current state of STYLE: Fixing and refactoring linting #22863, and the other checks we can probably wait for the CI)
  • Have the lint.sh that iterates over azure-pipelines.yml and run its commands

@jbrockmendel
Copy link
Member

But even with that we'd have more than 10 scripts.

My only real thought here is that I prefer a centralized ci/lint.sh to less-centralized. I have a hard enough time keeping track of what is checked where as it is.

@datapythonista
Copy link
Member Author

@vtbassmatt I'm testing the publishing of the docs from a branch in the pandas repo, and looks like that indeed fixes the secret variables problem. But I'm getting an error that the specified container doesn't exist. I created a container named web, but still the same error.

This is the container:
azure_storage_container

And you can see the error here: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=4427

2018-12-01T03:01:23.6821015Z ##[section]Starting: Publishing docs (Azure storage)
2018-12-01T03:01:23.6824359Z ==============================================================================
2018-12-01T03:01:23.6824504Z Task         : Command Line
2018-12-01T03:01:23.6824616Z Description  : Run a command line script using cmd.exe on Windows and bash on macOS and Linux.
2018-12-01T03:01:23.6824745Z Version      : 2.142.2
2018-12-01T03:01:23.6824854Z Author       : Microsoft Corporation
2018-12-01T03:01:23.6824979Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkID=613735)
2018-12-01T03:01:23.6825218Z ==============================================================================
2018-12-01T03:01:23.8249356Z Generating script.
2018-12-01T03:01:23.8313839Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/8cf84557-3f35-4912-8280-707aacf05035.sh
2018-12-01T03:01:44.2572732Z WARNING: The installed extension 'storage-preview' is in preview.
2018-12-01T03:01:47.2522240Z WARNING: uploading /home/vsts/work/1/s/doc/build/html/indexing.html
2018-12-01T03:01:47.7692495Z ERROR: The specified container does not exist. ErrorCode: ContainerNotFound
2018-12-01T03:01:47.7694905Z <?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
2018-12-01T03:01:47.7695666Z RequestId:8b38210c-d01e-0077-7022-894d0e000000
2018-12-01T03:01:47.7696273Z Time:2018-12-01T03:01:47.6226510Z</Message></Error>
2018-12-01T03:01:47.7696599Z Traceback (most recent call last):
2018-12-01T03:01:47.7697153Z   File "/opt/az/lib/python3.6/site-packages/knack/cli.py", line 197, in invoke
2018-12-01T03:01:47.7697639Z     cmd_result = self.invocation.execute(args)
2018-12-01T03:01:47.7698160Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/__init__.py", line 366, in execute
2018-12-01T03:01:47.7698452Z     cmd.exception_handler(ex)
2018-12-01T03:01:47.7699073Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/__init__.py", line 234, in new_handler
2018-12-01T03:01:47.7699376Z     handler(ex)
2018-12-01T03:01:47.7699845Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/__init__.py", line 177, in handler
2018-12-01T03:01:47.7700128Z     raise ex
2018-12-01T03:01:47.7700564Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/__init__.py", line 343, in execute
2018-12-01T03:01:47.7700851Z     result = cmd(params)
2018-12-01T03:01:47.7701305Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/__init__.py", line 182, in __call__
2018-12-01T03:01:47.7701598Z     return self.handler(*args, **kwargs)
2018-12-01T03:01:47.7702308Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/core/__init__.py", line 436, in default_command_handler
2018-12-01T03:01:47.7702870Z     result = op(**command_args)
2018-12-01T03:01:47.7703609Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/operations/blob.py", line 199, in storage_blob_upload_batch
2018-12-01T03:01:47.7704364Z     if_none_match=if_none_match, timeout=timeout)
2018-12-01T03:01:47.7705003Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/util.py", line 218, in wrapper
2018-12-01T03:01:47.7705366Z     return True, func(*args, **kwargs)
2018-12-01T03:01:47.7705945Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/operations/blob.py", line 185, in _upload_blob
2018-12-01T03:01:47.7706305Z     return upload_blob(*args, **kwargs)
2018-12-01T03:01:47.7706901Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/operations/blob.py", line 286, in upload_blob
2018-12-01T03:01:47.7707444Z     return type_func[blob_type]()
2018-12-01T03:01:47.7707938Z   File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/storage/operations/blob.py", line 279, in upload_block_blob
2018-12-01T03:01:47.7708231Z     return client.create_blob_from_path(**create_blob_args)
2018-12-01T03:01:47.7708733Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/blob/blockblobservice.py", line 463, in create_blob_from_path
2018-12-01T03:01:47.7709230Z     timeout=timeout)
2018-12-01T03:01:47.7709760Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/blob/blockblobservice.py", line 582, in create_blob_from_stream
2018-12-01T03:01:47.7710080Z     timeout=timeout)
2018-12-01T03:01:47.7710585Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/blob/blockblobservice.py", line 971, in _put_blob
2018-12-01T03:01:47.7710905Z     return self._perform_request(request, _parse_base_properties)
2018-12-01T03:01:47.7711432Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/common/storageclient.py", line 381, in _perform_request
2018-12-01T03:01:47.7711750Z     raise ex
2018-12-01T03:01:47.7712243Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/common/storageclient.py", line 306, in _perform_request
2018-12-01T03:01:47.7712548Z     raise ex
2018-12-01T03:01:47.7713076Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/common/storageclient.py", line 292, in _perform_request
2018-12-01T03:01:47.7713572Z     HTTPError(response.status, response.message, response.headers, response.body))
2018-12-01T03:01:47.7714756Z   File "/opt/az/lib/python3.6/site-packages/azure/multiapi/storage/v2018_03_28/common/_error.py", line 115, in _http_error_handler
2018-12-01T03:01:47.7715115Z     raise ex
2018-12-01T03:01:47.7715419Z azure.common.AzureMissingResourceHttpError: The specified container does not exist. ErrorCode: ContainerNotFound
2018-12-01T03:01:47.7716037Z <?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
2018-12-01T03:01:47.7716650Z RequestId:8b38210c-d01e-0077-7022-894d0e000000
2018-12-01T03:01:47.7717804Z Time:2018-12-01T03:01:47.6226510Z</Message></Error>
2018-12-01T03:01:47.8524094Z Documentation uploaded to https://pandas.blob.core.windows.net
2018-12-01T03:01:47.8535123Z ##[section]Finishing: Publishing docs (Azure storage)

Is web the right name of the container? Do I need to give permissions or something?

Btw, is it normal that if a command in the step fails, it continues and shows the job as success? Should I do a set -e, or there is another way to make it fail?

@vtbassmatt
Copy link
Contributor

vtbassmatt commented Dec 1, 2018 via email

RET=$(($RET + $?)) ; echo $MSG "DONE"

MSG='Check for incorrect sphinx directives' ; echo $MSG
! grep -R --include="*.py" --include="*.pyx" --include="*.rst" -E "\.\. (autosummary|contents|currentmodule|deprecated|function|image|important|include|ipython|literalinclude|math|module|note|raw|seealso|toctree|versionadded|versionchanged|warning):[^:]" ./pandas ./doc/source
invgrep -R --include="*.py" --include="*.pyx" --include="*.rst" -E "\.\. (autosummary|contents|currentmodule|deprecated|function|image|important|include|ipython|literalinclude|math|module|note|raw|seealso|toctree|versionadded|versionchanged|warning):[^:]" ./pandas ./doc/source
RET=$(($RET + $?)) ; echo $MSG "DONE"

MSG='Check that the deprecated `assert_raises_regex` is not used (`pytest.raises(match=pattern)` should be used instead)' ; echo $MSG
! grep -R --exclude=*.pyc --exclude=testing.py --exclude=test_testing.py assert_raises_regex pandas
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed an invgrep here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @h-vetinari, that was added later, and I didn't see it after merging from master. Well spotted, I fixed it.

@datapythonista
Copy link
Member Author

Thanks @vtbassmatt, I've got the '$web' as in your example, I think what's missing is enabling the static files.

I couldn't find where to activate it, but I saw the plan we've got is a free trial expiring in 3 weeks. As for now, we're just able to publish the dev documentation committed to master (and not the built documentation of every PR as initially planned), I think we can stay in GitHub pages.

So, I'm removing the part that builds and uploads the documentation in azure for now (it's still in travis). So, we should be able to merge this PR with all the linting, and I'll check in a new PR what can be done with the documentation.

@datapythonista
Copy link
Member Author

@jreback, all green now, can you take another look please?

@jreback
Copy link
Contributor

jreback commented Dec 2, 2018

@datapythonista can you rebase.

also can you point to a failed build (where the fail is on the liniting). E.g. I want to see how easy it is for a user to navigate to this. (maybe just put a lint error in here for it to fail)

@datapythonista
Copy link
Member Author

you can see how an error looks like here: #22854 (comment)

much easier to find the errors than in travis for sure :)

@jreback jreback added this to the 0.24.0 milestone Dec 2, 2018
@jreback
Copy link
Contributor

jreback commented Dec 2, 2018

lgtm merge on green

@datapythonista datapythonista merged commit 022f458 into pandas-dev:master Dec 3, 2018
@datapythonista
Copy link
Member Author

thanks for the review @jreback, and thanks a lot for all the help with this @vtbassmatt

echo "Benchmarks did not run, no changes detected"
fi
displayName: 'Running benchmarks'
condition: true
Copy link
Member

@gfyoung gfyoung Dec 3, 2018

Choose a reason for hiding this comment

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

Why did you add running benchmarks when the original issue was just to migrate lint.sh to Azure? IMO, this should have been considered in a separate PR.

Also, as your changes did not actually modify any Python benchmark code, it was not possible to review how it would look in the Azure logs. This log here from another PR (#23752) is unfortunately impossible to read:

https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=4580

Copy link
Member

@gfyoung gfyoung Dec 3, 2018

Choose a reason for hiding this comment

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

Also, it's somewhat debatable as to whether we even want to just run benchmarks when there's only a diff in asv_bench. We often ask people to run asv whenever there are changes to C and Cython code in general, just to make sure that no benchmarks were affected as a result of the changes.

source activate pandas-dev
ci/code_checks.sh doctests
displayName: 'Running doctests'
condition: true
Copy link
Member

@gfyoung gfyoung Dec 3, 2018

Choose a reason for hiding this comment

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

Hmm...IMO, I think running doctests should be a separate build entirely from linting. They are two semantically different things, and the separation would allow us to provide more semantically clear display names such as "Doctests" and "Linting" instead of the slightly vaguer mashup of "Checks_and_doc".

Copy link
Member

@gfyoung gfyoung Dec 3, 2018

Choose a reason for hiding this comment

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

I see there was a concern about not having to use an extra build for this due to resource constraints (the conversation is a very long in this PR...)? However, I'm not sure I fully understand...

Also, based on the conversation, are the master docs no longer being published on https://pandas-docs.github.io/pandas-docs-travis? In which case, we need to update the GITHUB_ISSUE template that we have here.

@datapythonista
Copy link
Member Author

@gfyoung I agree running benchmarks (and building the docs, which was implemented here), could be better addressed in separate PRs. Except that this PR took 2 months to be able to be ready, for different problems, and those things couldn't be implemented in parallel. And I didn't want to have them blocked for so long.

I don't understand your concerns.

The dev documentation is built in the same exact way as before, that wasn't touched.

I don't think anyone had a problem of running the linting in one of the tests builds. And now you don't want to have it with the doctests. We have around 12 of 14 builds that run the test suite with different configurations and for different platforms. I think having everything else in a separate build makes it very easy to know where to find things. This is not yet true for the docs, that we couldn't move them yet. And as the builds spend most of the time creating the environment and building pandas, I think it's much better to have a single build with everything that is not running the main tests, than having 10, because the linting is semantically different than the greps, than the docs, than the doctests, than the benchmarks, than the docstring formats...

Not sure which log is impossible to read. It takes me around 3 minutes to find the linting problems in travis, and it takes me around 10 seconds to find them in azure. I added screenshots and links on how things looked while they were broken in azure. And left them broken for more than a month.

It can make sense to run the benchmarks more often. But running them when the benchmarks itself, if you're happy to run them in the CI, is better than not running them, like until now.

I think this PR makes things much much better in the CI, and will save us lots of time. And I don't quite understand all the post-merge comments like if this was a mistake. Of course it'd be great for any of the things you propose, to open issues, so we can discuss there, and possibly implement then. And you can tag me, so I can share my experience.

@datapythonista
Copy link
Member Author

@gfyoung I think I misunderstood you regarding the logs difficult to read. I think you meant the benchmarks only, right? I created #24061 as there is a bug in the code, and line breaks are not being displayed.

@jreback
Copy link
Contributor

jreback commented Dec 3, 2018

just to be clear

running code checks (including doc tests) and the doc build is fine; this would include
a code check that the benchmarks actually are semantically correct not actually running them - which is not done in in this repo at all

rather in :
https://travis-ci.org/pandas-dev/pandas-ci

though this often breaks because of the timeout

@datapythonista
Copy link
Member Author

the implemented code check is a dry-run of the benchmark (and we are also linting the benchmarks code)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BLD: Use Azure pipelines for linting
10 participants