Add FjORD baseline #2431
Add FjORD baseline #2431
Conversation
Co-authored-by: Steve Laskaridis <[email protected]> Co-authored-by: Samuel Horvath <[email protected]>
jafermarq
added
the
summer-of-reproducibility
|
Hi @stevelaskaridis, @SamuelHorvath I started looking into the code but it is not following the expected file structure. The easiest way imo would be to, run the script in from the aforementioned directory. Instructions shown here. This is the procedure all other baselines have follow. Also, part of the tests check for that particular file structure to be followed (you can add more files if needed). There is nothing super special about what the script above does, but it's desirable for all baselines to follow a similar structure. Please ping me when I should look into the code again. |
|
Hey @jafermarq, thanks for the feedback. We hadn't realise that the template was to be that strictly followed. In its current form, our baseline implementation should now be closer to what you were looking for. Tests-wise, I am not sure the current failures are something on our side. But please let me know if it is something that we can fix. |
There was a problem hiding this comment.
Hi @stevelaskaridis, @SamuelHorvath
Thanks for making those changes. I was able to run the code, so that's great!!
Please find quite a few suggestions below (most of them are very minor and you can, if happy with it, just commit the suggestion. In summary these changes propose to:
- Add the remaining of the content to
pyproject.toml(so it matches that in the template) - Execute the code from the top-level dir and treat
fjordas a module -- this essentially requires making small adjustments to theimports. Some import paths could be shortened ifod/andutils/are outside their ownfjordsub dir. You'll find them all suggested below. The idea is for all baselines to run similar to:
#after poetry shell
python -m fjord.main <options-to-override-config-settings>- To keep things aligned, could you move your
setup.pyandrequirements.txtto the same directory as the readme?. - I suggested a simplification to the configs. It doesn't introduces changes. just keeps them a bit simpler (simpler to override from CLI too)
- Suggested changes to the readme. just to make the env setup a bit more self-contained (uniformity across baselines is ideal)
I applied on my side all the above changes to the code and configs, things were running fine.
It's great having a notebook! Could you move it to a directory called docs and placed at the same level as the readme (just for consistency with other baselines). Also, would it be possible to add the images to the readme in a directory named _static?
Pleas ping me when you'd like me to take another look and happy to discuss about the suggested changes further.
Co-authored-by: Javier <[email protected]>
Co-authored-by: Javier <[email protected]>
Co-authored-by: Javier <[email protected]>
Co-authored-by: Javier <[email protected]>
Co-authored-by: Javier <[email protected]>
stevelaskaridis
force-pushed
the
fjord-flwr
branch
from
36a049c
to
a02b83f
Compare
|
Hi @jafermarq, Thanks for the feedback. Please find below the requested and applied changes as a checklist:
|
There was a problem hiding this comment.
Hi @stevelaskaridis,
Thank you for applying those suggestions. I have listed a few more below. Probably the main two are:
- fix import in
utils/utils.py. - Install
fjordusingpython setup.py install(else conflict with poetry's.toml).
Finally, if you run ./dev/format-baseline.sh fjord (while sourced in your poetry env) you'll see several warning/errors (mostly in the form of D102 Missing docstring in public method) that are easy to fix for the most part. Then you'll need to run the tests ./dev/test-baseline.sh fjord. These two are needed before fjord can be merged. Happy to help if some are harder to fix.
Please ping me once you had a chance to look into the above. Happy to discuss these further.
Co-authored-by: Javier <[email protected]>
|
Hi @stevelaskaridis, Thanks for adding those changes. I tried running the code but although it starts, something crashes (w/o error message) when doing the first aggregation. The culprit is something added in commit a0ff259 |
|
Apologies for this @jafermarq. This should be fixed now. |
|
@stevelaskaridis , no worries. I made the following changes:
(After the above i re-run Let me know once you have some time to check those. Please ping me if you encounter some issues. Happy to help Edit: I have fixed the |
Options:
-h, --help show this help message and exit
--long-help more verbose help.
Master:
--init-hook=<code> Python code to execute, usually for sys.path
manipulation such as pygtk.require().
-E, --errors-only In error mode, checkers without error messages are
disabled and for others, only the ERROR messages are
displayed, and no reports are done by default.
--py3k In Python 3 porting mode, all checkers will be
disabled and only messages emitted by the porting
checker will be displayed.
-v, --verbose In verbose mode, extra non-checker-related info will
be displayed.
--ignore=<file>[,<file>...]
Files or directories to be skipped. They should be
base names, not paths. [current: CVS]
--ignore-patterns=<pattern>[,<pattern>...]
Files or directories matching the regex patterns are
skipped. The regex matches against base names, not
paths. [current: none]
--persistent=<y_or_n>
Pickle collected data for later comparisons. [current:
yes]
--load-plugins=<modules>
List of plugins (as comma separated values of python
module names) to load, usually to register additional
checkers. [current: none]
--fail-under=<score>
Specify a score threshold to be exceeded before
program exits with error. [current: 10.0]
-j <n-processes>, --jobs=<n-processes>
Use multiple processes to speed up Pylint. Specifying
0 will auto-detect the number of processors available
to use. [current: 1]
--limit-inference-results=<number-of-results>
Control the amount of potential inferred values when
inferring a single object. This can help the
performance when dealing with large functions or
complex, nested conditions. [current: 100]
--extension-pkg-allow-list=<pkg[,pkg]>
A comma-separated list of package or module names from
where C extensions may be loaded. Extensions are
loading into the active Python interpreter and may run
arbitrary code. [current: none]
--extension-pkg-whitelist=<pkg[,pkg]>
A comma-separated list of package or module names from
where C extensions may be loaded. Extensions are
loading into the active Python interpreter and may run
arbitrary code. (This is an alternative name to
extension-pkg-allow-list for backward compatibility.)
[current: none]
--suggestion-mode=<yn>
When enabled, pylint would attempt to guess common
misconfiguration and emit user-friendly hints instead
of false-positive error messages. [current: yes]
--exit-zero Always return a 0 (non-error) status code, even if
lint errors are found. This is primarily useful in
continuous integration scripts.
--from-stdin Interpret the stdin as a python script, whose filename
needs to be passed as the module_or_package argument.
Commands:
--rcfile=<file> Specify a configuration file to load.
--output=<file> Specify an output file.
--help-msg=<msg-id>
Display a help message for the given message id and
exit. The value may be a comma separated list of
message ids.
--list-msgs Generate pylint's messages.
--list-msgs-enabled
Display a list of what messages are enabled and
disabled with the given configuration.
--list-groups List pylint's message groups.
--list-conf-levels Generate pylint's confidence levels.
--list-extensions List available extensions.
--full-documentation
Generate pylint's full documentation.
--generate-rcfile Generate a sample configuration file according to the
current configuration. You can put other options
before this one to get them in the generated
configuration.
Messages control:
--confidence=<levels>
Only show warnings with the listed confidence levels.
Leave empty to show all. Valid levels: HIGH,
INFERENCE, INFERENCE_FAILURE, UNDEFINED. [current:
none]
-e <msg ids>, --enable=<msg ids>
Enable the message, report, category or checker with
the given id(s). You can either give multiple
identifier separated by comma (,) or put this option
multiple time (only on the command line, not in the
configuration file where it should appear only once).
See also the "--disable" option for examples.
-d <msg ids>, --disable=<msg ids>
Disable the message, report, category or checker with
the given id(s). You can either give multiple
identifiers separated by comma (,) or put this option
multiple times (only on the command line, not in the
configuration file where it should appear only once).
You can also use "--disable=all" to disable everything
first and then reenable specific checks. For example,
if you want to run only the similarities checker, you
can use "--disable=all --enable=similarities". If you
want to run only the classes checker, but have no
Warning level messages displayed, use "--disable=all
--enable=classes --disable=W".
Reports:
-f <format>, --output-format=<format>
Set the output format. Available formats are text,
parseable, colorized, json and msvs (visual studio).
You can also give a reporter class, e.g.
mypackage.mymodule.MyReporterClass. [current: text]
-r <y_or_n>, --reports=<y_or_n>
Tells whether to display a full report or only the
messages. [current: no]
--evaluation=<python_expression>
Python expression which should return a score less
than or equal to 10. You have access to the variables
'error', 'warning', 'refactor', and 'convention' which
contain the number of messages in each category, as
well as 'statement' which is the total number of
statements analyzed. This score is used by the global
evaluation report (RP0004). [current: 10.0 - ((float(5
* error + warning + refactor + convention) /
statement) * 10)]
-s <y_or_n>, --score=<y_or_n>
Activate the evaluation score. [current: yes]
--msg-template=<template>
Template used to display messages. This is a python
new-style format string used to format the message
information. See doc for all details.good-names
stevelaskaridis
force-pushed
the
fjord-flwr
branch
from
525465a
to
e3f1a06
Compare
stevelaskaridis
force-pushed
the
fjord-flwr
branch
from
e3f1a06
to
ba96b09
Compare
stevelaskaridis
force-pushed
the
fjord-flwr
branch
from
73f6e56
to
a6ce6ac
Compare
|
Hi @jafermarq, Thanks for the previous changes. I have either solved or omitted the remaining ones. Hope this now works as intended. A strange issue is that some pylint issues I could only see over github tests (despite using the poetry setup). The current structure tests will not pass as we don't need a |
|
@stevelaskaridis nothing to be done. Thanks! I'll run the experiments once and all should be good to merge 👍 |
stevelaskaridis
requested review from
tanertopal and
danieljanes
as code owners
There was a problem hiding this comment.
Looks great @stevelaskaridis @SamuelHorvath!! Thank you for contributing FjORD!
Issue
Description
Implements FjORD baseline.
Related issues/PRs
See issue #2035.
Proposal
Implementation of FjORD: Fair and Accurate Federated Learning under heterogeneous targets with Ordered Dropout (published at NeurIPS'21) for Flower's Summer of Reproducibility.
Explanation
The current PR implements FjORD baseline for summer of reproducibility. Specifically, we provide code for the ordered dropout implementation and replicate the results of Figure 5 of the paper.
Checklist
#contributions)Any other comments?