Need to decide what standalone domains go in rddlrepository and which in pyrddlgym (assuming there will be in pyrddlgym)

[ssanner]

For inclusion in PyRDDLGym, either: (1) No domains (everything in rddlrepository and all examples use rddlrepository). (2) Keep only the 10 domains shown in the animated GIFs in the README for PyRDDLGym. I vote for (1), but if you want some domains in PyRDDLGym then (2)... Ayal and Mike can vote and we go with the majority.

On Sun, Feb 4, 2024 at 8:53 PM Mike Gimelfarb @.***> wrote:

Assigned #242 https://github.com/ataitler/pyRDDLGym/issues/242 to @ssanner https://github.com/ssanner.

— Reply to this email directly, view it on GitHub https://github.com/ataitler/pyRDDLGym/issues/242#event-11697641502, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRA3JE47MP5T366MMFXHG3YSA3Q7AVCNFSM6AAAAABCZK3OCOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRGY4TONRUGE2TAMQ . You are receiving this because you were assigned.Message ID: @.***>

[mike-gimelfarb]

Sounds like you and Ayal are in agreement about (1). I concur that it will reduce the code maintenance by a lot, so I will be ok with it. Then we can remove the make() functionality, in fact the entire directory of environments, and the existing run scripts will need to be modified to use rddlrepository.

[mike-gimelfarb]

In any case, I will go ahead and start migrating the extra examples in pyrddlgym over to rddlrepo.

[ssanner]

OK, just keep in mind two requirements for domain/instance use outside of rddlrepository: (1) Anyone should be able to create, run, and iteratively edit their local domains without putting them in rddlrepository. (2) For the tutorial Colabs, we need the ability to create RDDL domains and instances from editable strings directly at run-time.

On Sun, Feb 4, 2024 at 9:47 PM Mike Gimelfarb @.***> wrote:

In any case, I will go ahead and start migrating the extra examples in pyrddlgym over to rddlrepo.

— Reply to this email directly, view it on GitHub https://github.com/ataitler/pyRDDLGym/issues/242#issuecomment-1926133176, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRA3JFDI4MM5G4BWA25DPTYSBB5PAVCNFSM6AAAAABCZK3OCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGEZTGMJXGY . You are receiving this because you were mentioned.Message ID: @.***>

[mike-gimelfarb]

For both (1) and (2) it will not be a problem, I don't think we removed any critical API functions from pyrddlgym to allow us to do it, so it will work as in prior colabs (there is likely just a change of package names that would be needed) and whatever workflows as it was done before. For (1), it actually gives me the idea to keep the current make() function if we remove the examples, but instead of specifying the domain name and instance identifiers from the examples, it could accept the rddl content directly. So pyRDDLGym.make(domain rddl as string, instance rddl as string, **other_kwargs). I'm still leaning a bit towards keeping at least some domains in pyRDDLGym, just because they would be minimal examples for running the test scripts without any added requirements. Then, we could keep make() and add make_from_string(). Does that make sense?

[ssanner]

I'm still leaning a bit towards keeping at least some domains in pyRDDLGym, just because they would be minimal examples for running the test scripts without any added requirements. Then, we could keep make() and add make_from_string(). Does that make sense?

Sure, I think it's OK to keep a few domains/instances in PyRDDLGym to demonstrate PyRDDLGym functionality for locally specified domains/instances.

Ideally these RDDL domains would also be included in rddlrepository.

On Sun, Feb 4, 2024 at 10:30 PM Mike Gimelfarb @.***> wrote:

For both (1) and (2) it will not be a problem, I don't think we removed any critical API functions from pyrddlgym to allow us to do it, so it will work as in prior colabs (there is likely just a change of package names that would be needed) and whatever workflows as it was done before. For (1), it actually gives me the idea to keep the current make() function if we remove the examples, but instead of specifying the domain name and instance identifiers from the examples, it could accept the rddl content directly. So pyRDDLGym.make(, , **other_kwargs). I'm still leaning a bit towards keeping at least some domains in pyRDDLGym, just because they would be minimal examples for running the test scripts without any added requirements. Then, we could keep make() and add make_from_string(). Does that make sense?

— Reply to this email directly, view it on GitHub https://github.com/ataitler/pyRDDLGym/issues/242#issuecomment-1926169844, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRA3JACUVL3HY4UZQWYCPTYSBG3VAVCNFSM6AAAAABCZK3OCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGE3DSOBUGQ . You are receiving this because you were mentioned.Message ID: @.***>

[mike-gimelfarb]

I'm looking at the colab notebook just to make sure I understand, but there is a line where the domains are loaded:

base_path = '/content/' myEnv = RDDLEnv(domain=base_path+'domain.rddl', instance=base_path+'instance.rddl')

So it seems even in the colab notebook it loads the examples from file.

Looking also at the API, the way the environment was coded, it doesn't actually allow loading anything with a pure string, that is not a path, so I am not sure how it was done in the notebooks. I'd say this is a limitation of the current/previous API that the user needs to specify only a path. I'd be willing to change it as it's not much work for me.

[mike-gimelfarb]

One option is in the RDDLEnv class, to have an optional field as_path (or similar), that the user can use to indicate whether the strings are paths or the actual rddl content. I don't know else this can be done. Do you have suggestions?

[ataitler]

The previous functionality will remain (specifying user made rddl files), it's a core function. I support (1) but in a slightly modified version. I think nothing should be stored in pyRDDLGym and it should rely on rddlrepository. However, I do think we should give a clean "built-in" examples accessible from pyRDDLGym, it does not mean that in the background pyRDDLGym cannot call upon rddlrepository for the information.

So I propose having everything only in rddlrepository, and have the examples in the gifs as "built-in" examples called with a single string: pyRDDLGym.make(example="powergen")

In my opinion pyRDDLGym.make(domain rddl as string, instance rddl as string, **other_kwargs) is not user friendly.

[mike-gimelfarb]

Just to understand clearly, you want to have no rddl content in pyRDDLGym, right? So all the examples including GIFs will be migrated to rddlrepository? Then we don't need a special make function. you can already load any example from rddlrepository with pyRDDLGym.make_from_rddlrepository("PowerGen", 1). The kwargs is optional, we can make the instance argument optional too. I can rename make_from_rddlrepository to make, because with 0 examples in pyrddlgym there is nothing to make.

[ataitler]

Mike not exactly. we should have one make function, the user should not care if it from rddlrepo or not, thats in the background. he can specify local custom made domains with path. he can install rddlrepo which basically gives him the same thing but from the rddlrepo folder, Or ask for one of the examples by string identifier and in the background pyrddlgym will call whatever it need (rddlrepo of course) and load it. The make method is a factory for RDDLEnv, the user should not care about anything else, and he definitly dont need make, make_from_rddlrepository, makefrom...

We should have a single make that gets domain path and instance number or a speciall string identifier for the "built in" examples.

[mike-gimelfarb]

Ok, but keep in mind that we need one function to do all of the following:

1. load from strings
2. load from file paths
3. load from example
4. load from rddlrepo Are you ok with that?

[mike-gimelfarb]

I was just proposing to separate them so that the intent is more clear, and the user semantically knows how the domain they want will be loaded. If done this way, it will be a lot more opaque, and likely less bug free. I don't mind doing it as you propose, but we will need at least 4 cases, if there are no others we missed in this chat.

[ataitler]

We need only load from path load from example

all the rest in included in load from path

[ataitler]

load from rddlrepo is basically up to the user to request the path from rddlrepo (why ask for the string, he asks for the path anyway).

And load from string we dont need, I dont see any place or recall any discussion about having that. Not to mention that it is a little ugly as you may ask the user to open and read huge text file and then pass them instead of just specifying path and load once without any need to pass them.

[mike-gimelfarb]

So I don't agree about loading from string not useful. Sometimes the user wants to perform a source transformation of an existing domain. This is inconvenient as you need to 1. save the content to disk, 2. load it separately from disk. In many IO intensive applications where the content is manageable in size, it's not optimal. It all depends on the application.

I argue it's in reverse. the user can always load the string with open(path, ...) as file, which by the way is how RDDLReader is already handling it. You load it into a string regardless. In fact, in cases as you propose, the user will typically use a buffered string reader, not a string. Then in our current setup, we need to provide the support for that and not the user.

[ataitler]

Then we will have to disagree on that. We don't support it now, it was never on the table, not sure why this functionality is even being proposed at this stage.

Your example is a corner case, more likely the user will create a distribution of rddl files in advance.

[mike-gimelfarb]

I don't understand about rddlrepo, sorry. There is a name identifier to load a domain, e.g. Wildfire_MDP_ippc2014, we definitely need support to just load that without the user needing to write many lines of code. I'm just proposing a set of one liners that can load a domain, not 3 lines that are difficult to lookup and remember. Otherwise, we don't need a make function at all.

[mike-gimelfarb]

In this case, the cleanest approach is just this: remove everything from pyrddlgym, then the user can just do env = pyRDDLGym.make(Wildfire_MDP_ippc2014, 1). That's all you need to setup an experiment.

[mike-gimelfarb]

That is just loading an example. We can disagree about loading strings for now, but it's fine, I agree it is not critical. Loading from path is identical pyRDDLGym.make(domain path, instance path). Again, you need kwargs so the user can pass whatever additional setting to RDDLEnv they want, but entirely optional.

[ataitler]

First, we don't need to provide support for every case in the world. We have a functionality that works, and we have good feedback about it. More is not necessarily better.

For the case for loading examples, yes that is all we need, not necessarily this unfriendly name, we can just have "wildfire" and pyrrdlgym will call the correct name from rddlrepo, but essentially yes. and also pyrddlgym.make(domain=path, instance=path) as we have and works

[mike-gimelfarb]

Ok, then we can remove all from pyrddlgym as it will be duplication anyway (it is very annoying to copy all the viz files). The make() function will load from repository. We can make the identifiers more user friendly, but let's do it in rddlrepository, I don't want to maintain a giant dictionary to map the names.. Finally, make() should also load from path, if I understand correctly, so we need logic to check if a valid path, etc. If yes, just load from path. Does this make sense?

[ataitler]

I agree we need additional args for the other settings no argument about that... we need to specify constraints vectorization etc... It's just the basic functionality of creating a basic env. You can also add arguments for "domain_str" and "instance_str" if you really wants to support rddl string content, I just think it does not add anything, and just make the whole thing a lot more confusing.

[mike-gimelfarb]

Ok, I agree at least for the time being. The idea of loading from string needs to be carefully designed, with buffers, etc. The idea of loading from string was suggested by Scott above, but when I looked at the colab, it still loads from path. So I guess it's not needed at all?

[mike-gimelfarb]

In any event, I will start making changes to the make() function, and we can adapt as needed over the next few days. Thanks for the inputs.

[ataitler]

Why a huge dictionary? only for 10 examples it is a 10 entries dictionary {"wildfire": wildfire_MDP_IPPC2011, ...} The viz should also be in rddlrepo and should be loaded from there, it worked until now as far as I know... We already have the logic to check for valid path no? has that been changed?

Scott meant the path thing, we never supported strings directly...

[ataitler]

I can do that also if you want it's no big deal.

[mike-gimelfarb]

I definitely wouldn't do a dictionary. Let's change this in rddlrepository instead. It's not elegant or maintainable design.

[ataitler]

I am not sure how to change it in rddlrepo as the identifiers have to be unique... Why do you object to a single small dic for that? it is just for examples (I am not sure we should even give more than one instance), and it should not change... its a preview..

[mike-gimelfarb]

Wait, why do you need a preview then? If the function loads any domain from rddlrepository, it's redundant completely.

[ataitler]

user friendliness... of course the functionality is in rddlrepo.

So why have examples at all in pyrddlgym (the discussion about where should they be stored and loaded from shoudl come after).

[mike-gimelfarb]

It's not though. If a user wants to use rddlrepo separately, they need a different key for the same domain. I propose pyRDDLGym.make(domain) for any domain in rddlrepo. The user needs nothing else.

[ataitler]

I am really against that, while we rely on rddlrepo (heavily) it is still a separate package and should be treated that way...

[mike-gimelfarb]

It's still a separate package.

[ataitler]

but if you do "pyRDDLGym.make(domain) " you practically eliminate that. the user does not see it anymore.

[mike-gimelfarb]

That's the point. It's still a separate package. If the user wants to load an example, they need to install it. The make() function just unifies it so that the user doesn't need to pass a bunch of objects from one to another. It's far too confusing to use both at the same time. Look, it's how open ai gym does it for many domains. if you want box2d, or atari, you don't need to deal with any of them.

[mike-gimelfarb]

If the user needs additional info about the domains, e.g. to figure out what's available, we must have documentation. I already modified error messages to provide suggestions. If they are an advanced user, they can call rrddlrepository directly for the information they need.

[ataitler]

I have been using gym for over 10 years now (give or take, from the first day it came out), they do it for built in envs. they are built in as they come with gym. third party envs are not like that, the registration process is a pain also. and they dont have "instances", so there are critical difference, and still we are "close" as we have a single make method, but the whole "environments" handling is very different in our case.

[mike-gimelfarb]

But for 99% of users, make() is more than enough.

[ataitler]

make should be enough for all the users! is should be a single point of access.

[mike-gimelfarb]

Yes, make() will be enough with my scheme. The average user doesn't need to work with two packages at once, just pyrddlgym. But they will need to install rddlrepository which is the backend for loading the examples. The only disadvantage is we will kill built in examples in pyrddlgym, but I'm comfortable with that. Pyrddlgym is the engine, and rddlrepo is for the domains, why not keep it distinct.

[ataitler]

unlike gym, we have a clear separation between the db of problems and the simulator. everything related to environment creation should be via make, everything related to problems, should be via rddlrepo

the purpose of "examples" is to give a preview of capabilities to someone who just installed pyrddlgym for the first time, he doesn't want to query rddlrepo, he just want to see something with his eyes in minimal amount of code.

[ataitler]

I am missing something, why killing the examples stored within pyrddlgym is a "disadvantage" and why installing rddlrepo as a dependency for pyrddlgym (that's what it means yes I agree) is a problem?

[mike-gimelfarb]

With what I propose, the average user will not need to query rddlrepo for any reason, unless they wish to add their own domains, or modify them, or they need to walk through them.

[mike-gimelfarb]

It's not an issue, I just wanted to make sure we're in agreement on that. I think we are. It was a side effect of my proposed solution with make().

[ataitler]

I think we agree yes, I don;t see it as side effect as we discussed it and understood the meaning, it is a design choise.

The average user, once he starts to work with pyrddlgym will have to query rddlrepo, I dont see any problem with that...

[mike-gimelfarb]

I think the advanced user should, not the average user, query rddlrepo directly.

[mike-gimelfarb]

I'd imagine people working with rddl will be in two groups. A is people who want to use it as a backtesting engine, they just need make(). B is people who write their own domains, write their own planning solutions on domains that we don't have already, etc.

[mike-gimelfarb]

In any case, I am suggesting we make the api as simple as possible with the least effort on the user's side. Then, if the user needs more than make(), e.g. mainly to register their own domains, then they have the rddlrepo api at their disposal. Even so, the user can register their own domain, then call make() on it.

[mike-gimelfarb]

We don't need a dictionary to map names if the standalone examples in rddlrepo are standardized either. There is already a CartPole, there is a RaceCar, etc. We need to improve the names of the domains there.