Farama-Foundation / stable-retro

Retro games for Reinforcement Learning
https://stable-retro.farama.org/
MIT License
163 stars 34 forks source link

Remove Travis-Ci and GCP related code #1

Closed zbeucler2018 closed 1 year ago

zbeucler2018 commented 1 year ago

There is a good amount of code related to Travis-ci and GCP (Google Cloud Platform) that is no longer needed. This code should be removed to reduce clutter and tech dept

onaclov2000 commented 1 year ago

Just to confirm we'd want to remove stuff like travis.py, .travis.yaml, those contain the GCP code (also check if anything calls them).

I suppose the relevant Dockerfile code too? (https://github.com/Farama-Foundation/stable-retro/blob/master/docker/linux/Dockerfile)

~Any other obvious things I am missing?~ I'll dig more but at least can point at the above items as necessary to remove.

zbeucler2018 commented 1 year ago

Yea I think that should be good

elliottower commented 1 year ago

Dockerfile sounds useful to keep IMO, as long as it’s not too hard to maintain I don’t see a problem having it

onaclov2000 commented 1 year ago

Yes, sorry, I was meaning there is GCP stuff in the dockerfile, I wasn't meaning to outright remove it.

onaclov2000 commented 1 year ago

Do we have a place we are putting the original travis related code? It seems like just flat out tossing it would maybe be a bad idea, we should make sure if we are pulling travis code out, it's going into dockerfiles (if docker is the path, or whatever CI ).

I.E. I was about to delete:

And realized if people didn't know that existed in the future we might have other issues and folks will have to do deep diving to find these things

pseudo-rnd-thoughts commented 1 year ago

Good point, we can always revert the code to the old version to find these changes so I won't worry about it

pseudo-rnd-thoughts commented 1 year ago

Is this done now?

onaclov2000 commented 1 year ago

Do we want to remove Travis files in the cores subdirectory? I haven't done that yet, but can pull them out and make a PR. (I dont know how many there are it may just be GBA but I can do a quick search)

onaclov2000 commented 1 year ago

There seemed to be a bit of overlap, I think someone else removed some Travis stuff, but inside the dockerfile there are still references to Google cloud stuff.

pseudo-rnd-thoughts commented 1 year ago

Do we want to remove Travis files in the cores subdirectory? I haven't done that yet, but can pull them out and make a PR. (I dont know how many there are it may just be GBA but I can do a quick search)

It seems to me that the original project, copy and pasted the emulators from projects which includes travis for testing.

As this was part of the original emulators, I'm in favor of not modifying any of the emulators (except for updating the whole project to a new version if possible)

There seemed to be a bit of overlap, I think someone else removed some Travis stuff, but inside the dockerfile there are still references to Google cloud stuff.

Which file are you referencing?

onaclov2000 commented 1 year ago

Yea I was actually wondering about whether we really wanted to modify the original projects code, I assume not tweaking it "should" allow an update easier if we decide we need to. I'm ok with that.

Example files: https://github.com/Farama-Foundation/stable-retro/blob/master/docker/linux/Dockerfile#L20

This has pip install google-auth google-cloud-storage pytest requests && \

Which we aren't using google cloud storage, nor google auth anymore.

I assume the same holds true more or less from the other dockerfiles in the docker folder.

Just to add a final note on the docker files, I know the one I added for windows works fine, but I'm actually not sure if the others work well/correctly/at all.

pseudo-rnd-thoughts commented 1 year ago

@onaclov2000 Could you try removing as much of those install as possible then we can close this issue

onaclov2000 commented 1 year ago

Pull Request submitted.

onaclov2000 commented 1 year ago

This can be closed.