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

Fixing CI #363

Merged
merged 55 commits into from
May 26, 2021
Merged

Fixing CI #363

merged 55 commits into from
May 26, 2021

Conversation

madhavajay
Copy link
Contributor

Description

Fixing CI.

Affected Dependencies

None

How has this been tested?

Locally and with CI.

Checklist

- Ran black black-nb mypy
- Removed python 3.5
- Fixed issue with MacOS Build using --features=-supports_dynamic_linker
- Updated Google DP to the latest commit
- Renamed Bazel target from bindings_test to pydp
- Updated pyproject.toml
- Fixed com_google_protobuf issue in WORKSPACE
- Disabled docker build on every commit / push
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

madhavajay added 11 commits May 11, 2021 18:59
- Ran pre-commit to fix lots of code issues
- Removed unused # type: ignore
- Updated setup.cfg with isort, mypy and flake8 rules
- Ran isort
- Disabled notebook tests
- Fixed matrix.os template variable in tests.yml
- Fix mypy issues with missing typeshed
- Fixing tests.yml
- Fixing issue with converting matrix.os to bazel config name
- Fixing install wheel on windows
- Fixing print version
@chinmayshah99
Copy link
Member

chinmayshah99 commented May 11, 2021

Does this also solve #358 ?

@chinmayshah99 chinmayshah99 changed the title Fixing CI [WIP] Fixing CI May 11, 2021
@madhavajay
Copy link
Contributor Author

Does this also solve #358 ?

Kind of. So it will build and install in CI, but really we should make sure that the wheel it creates can also install on any version of MacOS. There might be some extra tricks to making that happen, and we will likely need to add a second entry to the matrix to be sure:

macOS Big Sur 11.0 | macos-11.0
macOS Catalina 10.15 | macos-latest or macos-10.15

@madhavajay
Copy link
Contributor Author

I also think this should be done a different way:
https://github.com/OpenMined/PyDP/blob/dev/tests/notebooks/test_all_notebooks.py

But before refactoring it would be good to get a clear understanding of the goal? I would highly suggest running linting and checks before committing in pre-commit and then again in the linting step of CI, but currently the pre-commit checks aren't all passing because there are a lot of flake8 issues.

Try this:

$ pre-commit run --all-files
Found 216 errors in 29 files (checked 42 source files)

However, I think they should be handled as a seperate set of tasks so for now the key is just to get the tests passing and the build working so that we can do a Python 3.9 release.

Once these are passing for 3.9 I can add a nightly build to run the older versions like 3.6, 3.7 and 3.8 every night, and a simple file which can be used to trigger them manually by doing something like this:

$ date > tests/trigger/versions

@replomancer
Copy link
Member

replomancer commented May 14, 2021

I only investigated test_bounded_mean.py There may be some issues with other tests (they fail for me). Here's what I found so far.

This test:

def test_bounded_mean_int64(make_loaded_object):
x = make_loaded_object(5, 100000000, 5)
res = x.result()
print(x, type(x), res, type(res))
assert expect_near(5.0, res, 0.1)

means if you compute a dp mean of many many many fives, the result should be close to five.

Apparently for performance reasons, instead of building big lists of fives, an attempt is made here to load data:

if os.path.exists(dump_filepath):
# Search for data dump to import
data = Summary()
data.load(dump_filepath)
x.merge(data)
else:
# No data dump found, we have to init the alforithm from scratch
# Add entries into algorithm
example_list = [value] * size
for _ in range(iter):
x.add_entries(example_list)
# Dump the initialized algorithm data to retrieve in future tests
x.serialize().save(dump_filepath)

from here:

;type.googleapis.com/differential_privacy.BoundedMeanSummary

This file looks a little different when something useful is serialized here. Currently instead of having many many fives, BoundedMean probably gets nothing and produces random results between lower and upper bounds. This test passes after data file is deleted but it takes almost 2 minutes on my machine.

Speaking of lower and upper bounds. Maybe something builds differently for me or somehow I downloaded another C++ lib version but for me this is also broken: (this was a result of some dev changes I was missing)

x = BoundedMean(1.0, 0, 0, 10, dtype="int64")

i.e. I get

E       RuntimeError: LInf sensitivity has to be positive but is 0

because

        x = BoundedMean(1.0, 0, 0, 10, dtype="int64")

means

        # lower_bound same as upper_bound:
        x = BoundedMean(epsilon=1.0, lower_bound=0, upper_bound=0, l0_sensitivity=10, dtype="int64")

I also get the same sensitivity error from here:

bm1 = BoundedMean(1, 0, 1, 10)
bm2 = BoundedMean(1, 0, 1, 10)

I need to increase upper_bound for it to stop crashing.

I may have ideas/comments tomorrow.

@replomancer
Copy link
Member

replomancer commented May 15, 2021

(edit: this was from missing some dev changes)
Other fails I see also result from sensitivity set to zero (LInf sensitivity is computed from other parameters) e.g.

def test_basic(self):
example_data = [1, 5, 7, 9, 13]
epsilon = 1.0
delta = 0
lower_bound, upper_bound = 0, 16
bv = BoundedVariance(epsilon, delta, lower_bound, upper_bound, dtype="float")

Here

BoundedVariance(epsilon, delta, lower_bound, upper_bound, dtype="float") 

actually means

BoundedVariance(epsilon=epsilon, lower_bound=delta, upper_bound=lower_bound, l0_sensitivity=upper_bound, dtype="float") 

so both lower_bound and upper_bound are set to 0.

Similar story in TestBoundedStandardDeviation and TestBoundedSum.

@replomancer replomancer mentioned this pull request May 16, 2021
replomancer and others added 3 commits May 17, 2021 10:39
- Combined publish actions into a single github action
- Added nightly tests for older versions of python
- Updated some package meta-data
- Bumped version to 1.0.3
- Added poetry.lock file to speed up CI
@madhavajay madhavajay requested a review from chinmayshah99 May 17, 2021 11:50
Copy link
Member

@chinmayshah99 chinmayshah99 left a comment

Choose a reason for hiding this comment

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

LGTM! can you remove the tunnel ssh?

@madhavajay
Copy link
Contributor Author

Versions less than 3.9 are now building correctly and all tests are passing: https://github.com/OpenMined/PyDP/actions/runs/875497191

@chinmayshah99 chinmayshah99 changed the title [WIP] Fixing CI Fixing CI May 26, 2021
@chinmayshah99 chinmayshah99 merged commit 149d760 into OpenMined:dev May 26, 2021
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.

3 participants