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

Update Dockerfile to add Catalyst #616

Merged
merged 7 commits into from
Oct 1, 2019
Merged

Update Dockerfile to add Catalyst #616

merged 7 commits into from
Oct 1, 2019

Conversation

Scitator
Copy link
Contributor

Hi, just want to add Catalyst - high-level PyTorch framework for reproducible and fast R&D.

@rosbo
Copy link
Contributor

rosbo commented Sep 24, 2019

Hi @Scitator,

Can you add tests to prevent regression. Examples can be found under the /tests folder in this repo.

Thank you

@rosbo rosbo self-requested a review September 24, 2019 17:28
@rosbo rosbo self-assigned this Sep 24, 2019
@rosbo rosbo added the new-package Requests for installing new packages label Sep 24, 2019
@Scitator
Copy link
Contributor Author

Hi @rosbo

So minimal test case is done ;)
All other test are done during catalyst CI.

@rosbo
Copy link
Contributor

rosbo commented Sep 24, 2019

Hi @Scitator,

If you can add a simple smoke test that exercise a common path of catalyst that would be great.

Because we are installing 1000s of packages in this image, some dependencies may get reinstalled to a different version which may cause you package to fail inside the Kaggle image even though it works in your own CI (given the dependency may differ).

Thank you

@Scitator
Copy link
Contributor Author

@rosbo Would mnist test be enough?

tests/test_catalyst.py Outdated Show resolved Hide resolved
@Scitator
Copy link
Contributor Author

@rosbo Could you please check example now?

@Scitator
Copy link
Contributor Author

Hmm...why do you think tests have failed?

@rosbo
Copy link
Contributor

rosbo commented Sep 30, 2019

@Scitator

======================================================================

ERROR: test_version (test_catalyst.TestCatalyst)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/input/tests/test_catalyst.py", line 85, in test_version

    self.assertIsNotNone(catalyst.__version__)

NameError: name 'catalyst' is not defined

@rosbo
Copy link
Contributor

rosbo commented Sep 30, 2019

It seems like the installation probably failed. I will look at the build logs.

Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

The logs seems to suggest the package was successfully installed:
Sep 26 22:03:20 Successfully installed catalyst-19.9.5 crc32c-1.7 safitty-1.2.5: Successfully installed catalyst-19.9.5.

The image it ran the tests against is gcr.io/kaggle-images/python:PR-616-pretest. I will pull it and inspect it to see if the catalyst package is present.

criterion=criterion,
optimizer=optimizer,
loaders=loaders,
logdir=logdir,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to call some sort of Close method?

The tests shows a ResourceWarning:

unclosed file <_io.TextIOWrapper name='/working/logs/log.txt' mode='a' encoding='UTF-8'>

Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

Ah. I found the issue.

The package is properly installed on the image:

$ pip show catalyst
Name: catalyst
Version: 19.9.5
Summary: Catalyst. High-level utils for PyTorch DL & RL research.
Home-page: https://github.com/catalyst-team/catalyst
Author: Sergey Kolesnikov
Author-email: [email protected]
License: Apache License 2.0
Location: /opt/conda/lib/python3.6/site-packages
Requires: opencv-python, tensorboardX, matplotlib, scikit-image, crc32c, pandas, PyYAML, torchvision, numpy, tqdm, scikit-learn, safitty, torch, imageio, plotly, seaborn

$ python
>>> import catalyst
>>> print(catalyst.__version__)
19.09.5

You only need to add the missing import statement (see my inline code comment).

tests/test_catalyst.py Show resolved Hide resolved
@Scitator
Copy link
Contributor Author

Scitator commented Oct 1, 2019

@rosbo Looks like all checks have passed :)
unclosed file issue will be resolved in Catalyst 19.10 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-package Requests for installing new packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants