Fix linter and add pre-commit CI #193

younik · 2024-10-11T10:11:59Z

Apologies for the big PR; I can split it in two if it is too hassle to review.

This PR adds the pre-commit check on CI and fixes all the problems with linters.

To fix the problems with pyright:

I removed torchtyping, in favor of torch.Tensor only. In this way, we lost shape typing (which is a nice feature), but we can have static typing checks. I suggest documenting well the functions with the expected shape and adding assert on shape when necessary.
Fixing a likely bug in ReplayBuffer.add. Please check the fix as it is different from Small fixes #192
ReplayBuffer itself is a bit problematic because it can accept Trajectories, Transitions, and tuples of States. While Trajectories and Transitions inherit from Container, States doesn't. Also, the code has a bunch of if-else to handle the cases differently. Should States (or a new class StatesTuple) inherit from Container, and require for Container objects a complete interface so ReplayBuffer can be agnostic to the underline object? Or should we have different subclasses of ReplayBuffer? For simplicity, atm, I proposed to make States inherit from Container, but I suggest we think about it (e.g. an object tuple of states may be more appropriate to inherit from Container). I can address this in a future PR.
Trajectory should inherit from Generic and differentiate between DiscreteStates and States, since they have different methods and some codes expect one over the else. For simplicity, I didn't do it in this PR.

younik · 2024-10-12T16:36:50Z

src/gfn/containers/replay_buffer.py

+                assert isinstance(
+                    self.training_objects, (Trajectories, Transitions)
+                )  # TODO: becasue we use last_states... is it correct?


check this: look like self.cutoff_distance>=0 only when we are working with Trajectories and Transitions, but there is no real check for it during init. Should this branch work also with tuple of states?

younik · 2024-10-12T16:41:14Z

src/gfn/containers/trajectories.py

+        estimator_outputs = self.estimator_outputs
+        other_estimator_outputs = other.estimator_outputs


[these things are required for narrowing types by the checker, which cannot be done on self attributes]

Can you add a comment explaining this (and other similar decisions which might confuse a future developer if they don't know the underlying reason)?

Sure, make sense

younik · 2024-10-12T16:52:16Z

src/gfn/states.py


-class States(ABC):
+
+class States(Container, ABC):


check if this is okay

Having states inherit from Container does not change it's function at all, correct, but adds some handy save/load features and makes it automatically compatible with the buffers, correct?

Yes, exactly. For example, Container have load/save which is used in ReplayBuffer.

josephdviviano · 2024-10-18T13:38:45Z

Hi Omar thanks this is great.

Perhaps can you split this into two PRs - one for typing and one for the replay buffer?

I think the typing requires more investigation before we undo all of that work, but the replay buffer changes seem very important to resolve soon.

Thanks so much!

younik · 2024-10-18T14:08:13Z

Sure, I did it here: #202

saleml · 2024-10-19T08:25:09Z

Thanks Omar for initiating this important work.

If we were to indeed remove torchtyping in favor of torch.Tensor (which I am leaning towards, following the discussion we had ~10 days ago), I highly suggest we follow Joseph's recommendation of forcing the shapes of inputs, whenever a function takes tensors as inputs.

One way to do that is to create a repo-wide subclass of Exception that takes as input the tensor to check, the desiderata (ndim, shape, type... ?) and throws an error when the conditions are not met?

younik · 2024-10-19T11:13:59Z

One way to do that is to create a repo-wide subclass of Exception that takes as input the tensor to check, the desiderata (ndim, shape, type... ?) and throws an error when the conditions are not met?

This is a very good idea, indeed.

I will do the work in other smaller PRs (drop torch typing, fix other typing errors, ...) for the sake of review.

josephdviviano · 2024-10-24T19:32:59Z

Hey Omar just a heads up - doing the review now but looks like you might have some merge conflicts to resolve as well. I like the plan stated above by you two RE: removing torchtyping.

younik · 2024-10-24T19:35:53Z

Hey Omar just a heads up - doing the review now but looks like you might have some merge conflicts to resolve as well. I like the plan stated above by you two RE: removing torchtyping.

Hey @josephdviviano, yes, please check #204. The plan is to break this PR into pieces, so I will close it.

josephdviviano

OK awesome PR - thanks - I have a few requests (sorry one of them i s super annoying -- the nature of a PR like this).

Really appreciate your attention to detail.

.pre-commit-config.yaml

src/gfn/containers/replay_buffer.py

josephdviviano · 2024-10-24T19:49:15Z

src/gfn/gflownet/sub_trajectory_balance.py


 from gfn.containers import Trajectories
 from gfn.env import Env
 from gfn.gflownet.base import TrajectoryBasedGFlowNet
 from gfn.modules import GFNModule, ScalarEstimator

-ContributionsTensor = TT["max_len * (1 + max_len) / 2", "n_trajectories"]


In this case, I think the notes will be particularly important.

src/gfn/gym/helpers/test_box_utils.py

josephdviviano · 2024-10-24T19:53:17Z

src/gfn/states.py


-class States(ABC):
+
+class States(Container, ABC):


Having states inherit from Container does not change it's function at all, correct, but adds some handy save/load features and makes it automatically compatible with the buffers, correct?

testing/test_gflownet.py

tutorials/examples/train_line.py

josephdviviano · 2024-10-24T20:06:34Z

@younik please use my notes from this review in any future PR, I don't want to have to re-do it once more.

younik

Apologies for making you review this big one.
If this PR looks good to you, we can move this forward and add the asserts (and docs) on shape immediately after. Otherwise, we can move the smaller one forward (I will integrate the comments here that are related to that one).

.pre-commit-config.yaml

src/gfn/containers/replay_buffer.py

younik · 2024-10-24T21:55:57Z

src/gfn/containers/trajectories.py

+        estimator_outputs = self.estimator_outputs
+        other_estimator_outputs = other.estimator_outputs


Sure, make sense

younik · 2024-10-24T22:00:26Z

src/gfn/gflownet/base.py

@@ -124,8 +131,8 @@ def get_pfs_and_pbs(
        fill_value: float = 0.0,
        recalculate_all_logprobs: bool = False,
    ) -> Tuple[
-        TT["max_length", "n_trajectories", torch.float],
-        TT["max_length", "n_trajectories", torch.float],
+        Tensor,


Yes, make sense. There is #204 which does it with asserts. As we briefly discussed last time, I also believe it makes sense to add the information to the documentation. I will do it in #204

younik · 2024-10-24T22:03:23Z

src/gfn/states.py


-class States(ABC):
+
+class States(Container, ABC):


Yes, exactly. For example, Container have load/save which is used in ReplayBuffer.

testing/test_gflownet.py

younik · 2024-10-25T16:40:25Z

I open it again and make it a draft, otherwise, it doesn't get updated.

josephdviviano · 2024-10-30T19:52:07Z

Hey @younik - I assume this is now safe to close?

younik · 2024-10-30T21:24:15Z

Hey @younik - I assume this is now safe to close?

Yes, I close it.

trying fixing linter

615007e

josephdviviano self-requested a review October 11, 2024 15:02

younik added 7 commits October 11, 2024 23:09

use pyright CI

4dcd580

use latest

68b5c04

fix pyright

28e25b8

restore full pre-commit

c947a5c

add pytest

0722f65

install all the deps

7d800b3

remove torch typing

16291d0

younik marked this pull request as ready for review October 12, 2024 17:04

younik commented Oct 12, 2024

View reviewed changes

hyeok9855 mentioned this pull request Oct 18, 2024

Issues in ReplayBuffer #201

Open

saleml assigned saleml and unassigned saleml Oct 19, 2024

saleml self-requested a review October 19, 2024 08:25

younik closed this Oct 24, 2024

josephdviviano requested changes Oct 24, 2024

View reviewed changes

younik commented Oct 24, 2024

View reviewed changes

fix circular import

01d3424

josephdviviano reopened this Oct 25, 2024

josephdviviano closed this Oct 25, 2024

younik added 2 commits October 25, 2024 18:24

rever pre-commit update

bc844a1

fix main branch name

220eeb9

younik reopened this Oct 25, 2024

younik marked this pull request as draft October 25, 2024 16:41

revert HyperGrid -> Box

a2d4a6b

josephdviviano mentioned this pull request Oct 28, 2024

test flowmatching gflownet bug? #206

Open

younik mentioned this pull request Oct 28, 2024

Fix formatting #207

Merged

younik closed this Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix linter and add pre-commit CI #193

Fix linter and add pre-commit CI #193

younik commented Oct 11, 2024 •

edited

Loading

younik Oct 12, 2024

younik Oct 12, 2024

josephdviviano Oct 24, 2024

younik Oct 24, 2024

younik Oct 12, 2024

josephdviviano Oct 24, 2024

younik Oct 24, 2024

josephdviviano commented Oct 18, 2024

younik commented Oct 18, 2024

saleml commented Oct 19, 2024

younik commented Oct 19, 2024

josephdviviano commented Oct 24, 2024

younik commented Oct 24, 2024

josephdviviano left a comment

josephdviviano Oct 24, 2024

josephdviviano Oct 24, 2024

josephdviviano commented Oct 24, 2024

younik left a comment

younik Oct 24, 2024

younik Oct 24, 2024

younik Oct 24, 2024

younik commented Oct 25, 2024

josephdviviano commented Oct 30, 2024

younik commented Oct 30, 2024

		estimator_outputs = self.estimator_outputs
		other_estimator_outputs = other.estimator_outputs

Fix linter and add pre-commit CI #193

Fix linter and add pre-commit CI #193

Conversation

younik commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephdviviano commented Oct 18, 2024

younik commented Oct 18, 2024

saleml commented Oct 19, 2024

younik commented Oct 19, 2024

josephdviviano commented Oct 24, 2024

younik commented Oct 24, 2024

josephdviviano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephdviviano commented Oct 24, 2024

younik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younik commented Oct 25, 2024

josephdviviano commented Oct 30, 2024

younik commented Oct 30, 2024

younik commented Oct 11, 2024 •

edited

Loading