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

Approx_RL main notebook update #544

Merged

Conversation

IsaevRustam
Copy link
Contributor

@IsaevRustam IsaevRustam commented Oct 27, 2024

Changes Made

  1. Refactored Wrappers

    • Replaced the deprecated gymnasium.wrappers.frame_stack with gym.wrappers.stateful_observation.FrameStackObservation to resolve the following error:
      module 'gymnasium.wrappers' has no attribute 'frame_stack'
    • Ensures compatibility with the latest Gymnasium version.
  2. Fixed Image Display Issues

    • Updated image handling logic to ensure proper display of all images.
  3. Added Flexible Logging with logger.py

    • Introduced logger.py with the Logger class, providing options to log to TensorBoard or use matplotlib.pyplot (plt) for visualization.

    • Usage Example:

      from logger import Logger
      
      # To log to TensorBoard
      logger = Logger(use_tensorboard=True, log_dir='logs/')
      
      # To log using matplotlib
      logger = Logger(use_tensorboard=False)
      
      logger.log_loss(loss_value, step)
      logger.log_grad_norm(grad_norm_value, step)
      logger.log_mean_reward(mean_reward_value, step)
      logger.log_initial_state_v(initial_v_value, step)
      
      # If using matplotlib for logging
      logger.plot()
      
      # Close the logger
      logger.close()

Notes

  • These updates enhance project stability and user experience. Aims to assist students who face issues with environment setup by providing a well-prepared notebook version that works good in Google Colab when it is solved correctly.
  • The new logger.py supports different environments, like Google Colab, simplifying visualization for students without complex setups.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@dniku
Copy link
Collaborator

dniku commented Oct 27, 2024

Thanks for the PR.

I would appreciate a detailed description if it wasn't hallucinated. E.g. (1) ("Upgraded all project dependencies to their latest versions") is definitely not in this PR.

Please update the PR description so it corresponds to reality. Deleting that auto-generated summary is fine.

@IsaevRustam
Copy link
Contributor Author

IsaevRustam commented Oct 27, 2024

Deleting that auto-generated summary is fine.

Thank you for the response. After reviewing it with fresh eyes, I realized there were no updates to libraries or reqs dependencies in the final PR.

I was dealing with an error related to the ALE namespace and was solving dependencies problems with this issue. I was solving deps errors and so on with gym and gymnasium (with ALE namespace) in different branch, but after few evenings I found a simple solution:

gym.register_envs(ale_py)

This caused the cursor bot to include those changes in the summary, even though they weren’t part of the final PR. My bad, didn't check it properly.

P.S. Updated PR description. Thanks for remarks.

@IsaevRustam
Copy link
Contributor Author

Hey!

Just recognized about this PR and its not closed or merged yet.

Should I close this PR or mb draft it?

@dniku
Copy link
Collaborator

dniku commented Dec 1, 2024

Hi again — apologies for the delay and thanks for your contribution!

There are a few more things I'd like to ask you to do before merging this:

  1. Please have another look at the PR description — it still contains (2) which does not appear to be in this PR.
  2. Please update the _debug.ipynb notebook similarly as well.
  3. ...and the comments below.

Side note: it looks like Github's notebook diff has been broken this entire time for me. This is the only diff it has been showing to me:

image

I've reviewed your changes using the raw JSON diff.

@IsaevRustam
Copy link
Contributor Author

IsaevRustam commented Dec 9, 2024

Please have another look at the PR description — it still contains (2) which does not appear to be in this PR.

Now added correct paths for images in notebook, dont know why it wasnt showed it first commit.

All images are now be like:

![img](https://github.com/yandexdataschool/Practical_RL/raw/master/yet_another_week/_resource/exp_replay.png)

This way, even without downloading the entire repo, you’ll see the images to help you build the correct network.

@IsaevRustam
Copy link
Contributor Author

IsaevRustam commented Dec 9, 2024

Please update the _debug.ipynb notebook similarly as well.

I updated the paths in the debug notebook, but it looks like I changed the entire file. Locally, my Git showed more lines as changed. It behaved the same way for the main notebook, but it displayed correctly on GitHub (as u can see). However, for the debug notebook, it’s behaving differently for some reason. Please trust that I only updated the paths and imports :)

@IsaevRustam
Copy link
Contributor Author

Just realized that all my commits were made using work credentials, and the account isn’t even displayed properly on github. This rr.isaev is me, and I hope this won’t have any significant impact. Do I need to reset and update the authorship? It seems like this can be done using --amend --reset-author, but I’ve never experimented with it before.

Copy link
Collaborator

@dniku dniku left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

  1. It's not a problem that the diff for the debug notebook is large — that's only the case because for some reason the indentation was changed. Looks like this time around Github notebook diff is correct, so I can see the actual changes.
  2. Are you sure there is something wrong with importing test? I just tested this in IPython and, as expected, import test fails with ModuleNotFoundError.
  3. If you are sure, please make sure you move the original file, not duplicate it.
  4. Don't worry about the commit authorship.

The rest looks good to me. Thanks again!

week04_approx_rl/homework_pytorch_main.ipynb Outdated Show resolved Hide resolved
@IsaevRustam
Copy link
Contributor Author

Are you sure there is something wrong with importing test?

Here’s what happens:

  1. In a fresh Google Colab session:
image
  1. In a clean Conda environment:
image
  1. When using wget to place a test file in the tests folder in Google Colab:
image

For example, if I run the following commands:

mkdir tests/
wget -q .../compute_td_loss.py -P tests/

I encounter an import error later with

from tests.compute_td_loss import test_compute_td_loss_double

Locally, I got an error only when including test and if dir is named tests everything is ok, but in Colab, I can import both test and tests without issues, so i got errors later with importing compute_td_loss. Am I missing something here, or is this behavior unnecessary/avoidable and we can keep dir tests?

Copy link
Collaborator

@dniku dniku left a comment

Choose a reason for hiding this comment

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

I see — the existence of test and tests modules is a bit annoying.

Thanks for you contribution — all LGTM.

@dniku dniku merged commit dfe3aa2 into yandexdataschool:master Dec 23, 2024
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.

2 participants