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

Added __version__ to Flower #952

Merged
merged 12 commits into from
Dec 28, 2021
Merged

Conversation

sisco0
Copy link
Contributor

@sisco0 sisco0 commented Dec 20, 2021

Added __version__ using importlib.metadata to retrieve installed package information. This pull request accomplishes #883 duty.

A screenshot of the testing process is attached below.
image

Added __version__ using importlib.metadata to retrieve installed package information
@tanertopal
Copy link
Member

@sisco0 Many thanks for your contributions. I can recommend to run the dev/test.sh script locally. It should allow you to see most errors which otherwise are discovered in the CI.

Copy link
Member

@tanertopal tanertopal left a comment

Choose a reason for hiding this comment

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

@sisco0 just did some digging and a good solution seems to be: python-poetry/poetry#2366 (comment) as importlib.metadata is only available after Python 3.8. We still support Python 3.7 though so we would need a solution which can handle it too. Would you like to adjust your approach?

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 22, 2021

I would do it without inconvenience by today by using setup tools instead of importlib.

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 23, 2021

Help is needed as I could accomplish this task locally by using 3.7.9 but for some reason, the same result is not obtained when using the Github Actions test, we are going to the incorrect branch of the comparison.

Edit
Ubuntu 20.04 comes with Python 3.8 preinstalled, could that be the reason? I would use Act to run the Github Actions locally and would come up with an answer.

@danieljanes
Copy link
Member

@sisco0 I took the liberty to update the PR with the latest changes from main, but Pylint is still failing: https://github.com/adap/flower/runs/4636866489?check_suite_focus=true

test_core runs on Ubuntu 20.04, but it uses GitHub Actions to test against Python 3.6 - 3.9 (https://github.com/adap/flower/blob/main/.github/workflows/flower.yml). I don't think the Python version is an issue here given that test_core also fails on Python 3.8.3.

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 26, 2021

We are not using an env in the Github Actions environment, I am trying configurations locally with act but I could not find a way yet.
On that path, I am trying to use another image as a base and see what happens.

Edit: As the error seems to come from PyLint we must look into that, this must be the key. Trying to go on with # pylint: disable=import-error as shown in a pylint conditional import Google search.

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 26, 2021

image
Seems promising locally when using Python 3.9.5, let me check what happens on the sorting.
Used a miniconda environment with Python 3.7.9 in it and fixed locally, waiting for the CI to accomplish the tasks.
I attach evidence of the local victory.
image
image

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 26, 2021

The good news is that the tests seem to pass now by the import statements, the bad news is that we had a line length violation. The new good news is that we already solved this by using sectioning for PyLint directives. Double good news is fine 🚀.

Copy link
Member

@tanertopal tanertopal left a comment

Choose a reason for hiding this comment

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

@sisco0 Amazing! Thank you a lot for this! We can merge it as is but one more improvement I would suggest would be to add a test for this.

My suggestions would be to have a __init___test.py with e.g. this test:

def test_version() -> None:
    """Tests if version is correctly imported."""
    # Execute
    from flwr import __version__  # pylint: disable=import-outside-toplevel

    # Assert
    assert isinstance(__version__, str)

wdyt?

@tanertopal tanertopal merged commit 3ccdc3a into adap:main Dec 28, 2021
@tanertopal
Copy link
Member

@sisco0 Merged! Thank you!

@sisco0
Copy link
Contributor Author

sisco0 commented Dec 28, 2021

Adding the test would be a good point for sure. I would try this during tomorrow but if someone wants to continue on this before me I would create an issue.

@sisco0 sisco0 mentioned this pull request Dec 28, 2021
@sisco0 sisco0 deleted the feat/add-version-to-flwr branch December 28, 2021 18:35
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.

None yet

3 participants