Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2020

Fixes #764.
The versions are the current versions of the respective packages.
I used all loggers in offline mode and they all seemed to work. Not sure how exhaustive the test should be.

@williamFalcon
Copy link
Contributor

thanks! why do we need a separate requirements? we already have a different one in tests.

@ghost
Copy link
Author

ghost commented Feb 7, 2020

ah thanks didn't know about that. The problem is, that we need to specify versions of optional packages (e.g. loggers) somewhere. More as a reference for users than a file for installing requirements.

@Borda
Copy link
Collaborator

Borda commented Feb 7, 2020

@williamFalcon as a user you do not want to install all testing packages, just those you need to run e.g. special logger... we have a warning message if you ale loading logger for which you do not have a package installed, but it is missing min version...

Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls copy versions from tests/requirements.txt with minimal versions
then in tests/requirements.txt imports these extras like -r ../requirements-extra.txt

@ghost
Copy link
Author

ghost commented Feb 10, 2020

@Borda only test_tube had a minimal version. Copied that, added -r ../requirements-extra.txt and removed the loggers from test/requirements.txt

@Borda
Copy link
Collaborator

Borda commented Feb 10, 2020

@Aljo-Rovco it would be great contributions to find out what is the min versions to pass all tests (you can try something like divide an conquer strategy =)
You even do not meet to run all tests, just tests/test_loaders.py

@ghost
Copy link
Author

ghost commented Feb 12, 2020

will deal with this either today or tomorrow

@ghost
Copy link
Author

ghost commented Feb 13, 2020

@Borda I assume you meant tests/test_logging.py?

@ghost
Copy link
Author

ghost commented Feb 13, 2020

I increased the amount of data that is used when running the tests (from 0.01 to 0.05), because otherwise the log method in the wandb-logger is not called. And if that method is not called, the tests would run fine with wandb==0.8.14. I increased the number for the other loggers as well, just to be on the save side

@ghost
Copy link
Author

ghost commented Feb 13, 2020

@Borda could you have a look at the failing checks? Not quite sure how to fix them

@Borda
Copy link
Collaborator

Borda commented Feb 13, 2020

lists of files in version control and sdist do not match!
suggested MANIFEST.in rules:
  include *.txt
missing from sdist:
  requirements-extra.txt

@Aljo-Rovco you need to include or exclude the new file in MANIFEST.ini

@Borda
Copy link
Collaborator

Borda commented Feb 14, 2020

it seems there is an ussie with wandb may you have look at it...

wandb.run_manager.LaunchError: W&B process failed to launch, see: C:\Users\RUNNER~1\AppData\Local\Temp\wandb-debug.log

@ghost
Copy link
Author

ghost commented Feb 14, 2020

had a look, but not sure how to solve it.
line 1126 in wandb/run_manager.py (stdout_read_file = os.fdopen(stdout_read_fd, 'rb')) throws OSError: [WinError 6] The handle is invalid. Not sure why. Could it be the location of the tempdir that is used for logging?
I don't have access to a windows machine, so I cannot recreate/test it.

@Borda
Copy link
Collaborator

Borda commented Feb 14, 2020

it was working before so let's try to increase the wandb version...

@ghost
Copy link
Author

ghost commented Feb 14, 2020

I think it was working before because the test didn't test much. It just created a logger:

def test_wandb_logger(tmpdir):
    """Verify that basic functionality of wandb logger works."""
    tutils.reset_seed()

    wandb_dir = os.path.join(tmpdir, "wandb")
     _ = WandbLogger(save_dir=wandb_dir, anonymous=True) 

All other tests actually trained a model

@ghost
Copy link
Author

ghost commented Feb 14, 2020

also, isn't the last version used in the CI testing?

@Borda
Copy link
Collaborator

Borda commented Feb 14, 2020

also, isn't the last version used in the CI testing?

add line in .github/workflow/ci-testing.yml

python -c "req = open('requirements-extra.txt').read().replace('>', '=') ; open('requirements-extra.txt', 'w').write(req)"

just belo the changing requirements.txt so we have the min version propoerly tested

@Borda
Copy link
Collaborator

Borda commented Feb 14, 2020

I think it was working before because the test didn't test much. It just created a logger:
...

I would say you can try to fix it here or remove the new test and open issue where you describe the problem along with your proposed test... I would suggest to to to the second option so we keep the PR simple... sure you can take the new issue and solve it :] @Aljo-Rovco

Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Borda Borda added feature Is an improvement or enhancement good first issue Good for newcomers labels Feb 17, 2020
@Borda Borda added the ready PRs ready to be merged label Feb 17, 2020
@Borda
Copy link
Collaborator

Borda commented Feb 17, 2020

@Aljo-Rovco pls create an issue with suggesting the tests you had here for wandb so we do not forget it and keep everything well tested :]

@Borda Borda changed the title added requirements-extra.txt for logger dependencies separate requirements for logger dependencies Feb 17, 2020
@ghost
Copy link
Author

ghost commented Feb 20, 2020

Sorry, was busy. Will do

@ghost
Copy link
Author

ghost commented Feb 20, 2020

created a new issue: #906

@williamFalcon williamFalcon merged commit 9eb1907 into Lightning-AI:master Feb 21, 2020
@Borda Borda mentioned this pull request Feb 22, 2020
@Borda Borda added this to the 0.7.0 milestone Mar 7, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* added file that contains information on the minimal versions needed for the supported loggers

* copied minimal version, combined files, deleted duplicates

* sorted functions in tests/test_loggers.py to be consistent

* expanded wandb logging test; added minimal versions for requirements-extra.txt; increased the amount of training data that is used for tests

* formatting

* added requirements-extra.txt to MANIFEST.in

* reverted wandb test; ensured minimal version for dependencies in requirements-extra.txt in ci-testing.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Is an improvement or enhancement good first issue Good for newcomers ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wandb logging does not work - log method called on the wrong object (?)

3 participants